perf: EXPOSED-204 Performance problem with getConnection() #1943
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e5l
approved these changes
Dec 14, 2023
| * | ||
| * This should only hold a non-null value if [connectsViaDataSource] has been set to `true`. | ||
| */ | ||
| internal var dataSourceIsolationLevel: Int? = null |
There was a problem hiding this comment.
I would try to avoid nullable fields because of boxing, but it shouldn't be a problem here
There was a problem hiding this comment.
@e5l I didn't consider the implications of Int? and boxing.
Since the goal of this PR is performance, it's an easy change to avoid the null entirely.
Please let me know if this is ok to go.
Override options include: - Using a cache of the datasource values to compare against per-transaction values - Getting the actual connection value to compare
In spite of reused connections from a connection pool instance, the lazy property initializer of a new database connection attempts to reset the transaction isolation level. This occurs regardless of whether the connection pool's set level has been overriden by a new value setting in DatabaseConfig or on the transaction itself. A flag in Database is now set if a DataSource is used to setup connections. On the first connection, this flag ensures that the level set by the DataSource is retrieved and cached. This avoids redundant resets in future connections unless the DataSource value has been overriden by DatabaseConfig or per-transaction values.
Rename new properties. Replace const integer value with a private flag that is passed from ThreadLocalTransactionManager to ThreadLocalTransaction instance. Edit tests to also check stored value in TransactionManager, in the event users access this property directly, before the data source value is retrieved and cached.
Switch unitialized Database.dataSourceIsolationLevel from nullable to -1.
bog-walk
force-pushed
the
bog-walk/fix-connection-performance
branch
from
27f3a01
to
1057508
Compare
This was referenced Dec 19, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In spite of reused connections from a connection pool instance, the lazy property initializer of a new database connection attempts to reset the transaction isolation level. This occurs regardless of whether the connection pool's set level has been overriden by a by a new value setting in
DatabaseConfigor on thetransaction {}itself.A flag in Database is now set if a
DataSourceis used to setup connections.On the first connection, this flag ensures that the level set by the DataSource is retrieved and cached. This avoids redundant resets in future connections unless the DataSource value has been overriden by
DatabaseConfigor per-transaction values.