-
Notifications
You must be signed in to change notification settings - Fork 934
[WIP] Intern string metadata in Session Factory #1503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
For this kind of change, adding a setting changing (even slightly) the behavior in many places, I would rather have some practical measurement of the gain. Do you think you could benchmark what impact this PR and its different option values have on both spatial and temporal performances? (Memory usage and execution time.) You may try to see if it has a significant impact on Frans's benchmark, but I am not sure this benchmark is adequate for this. |
@fredericDelaporte Ok. Give me a few days. I will check out Frans's benchmark but I don't expect any significant memory drops in single factory scenario. Also I don't think it will affect execution time - my changes affect performance of session factory creation only which is not measured in those tests. But will see... |
I played with interning using mappings from RawDataBencher and a bit disappointed - with 2 app domains and 3 session factories in each app domain (so 6 session factories in total) savings between minimum(basically current code) and maximum interning is about 300 Kb. There is no noticeable performance penalties but the gain is also insignificant in the current state of PR. Pushing my changes just for academic purposes : ) But I noticed a few things that I think should be addressed: nhibernate-core/src/NHibernate/Mapping/Table.cs Lines 53 to 56 in d372ee3
nhibernate-core/src/NHibernate/Mapping/Table.cs Lines 29 to 30 in d372ee3
This UniqueInteger is used in alias generation and that leads to different aliases for the same table in different session factories. And so alias interning is completely useless and dangerous.
I think we should make 2) nhibernate-core/src/NHibernate/Cfg/XmlSchemas.cs Lines 10 to 14 in d372ee3
I personally think that both variables should not be static and scoped to session factory creation. But if you think that static caching is useful - hbm schema needs to be lazy initialized. Let me know what you think - I can try to prepare a PRs for both points I mentioned if you agree with me. |
|
I meant the case multiple apps and multiple databases, same mapping for all databases. So for App1 Database1 was accessed first and session factory is generated with index 1. But for App2 Database2 was first accessed so for Database1 session factory is generated with index 2. |
Shall we close this? |
Not yet. Let me play a little bit with stable table aliases (#1517) |
Shall we close this now? |
Ok |
String interning is pretty powerful concept but it needs to be used carefully.
Intern methods should not be used in frequently called methods or in short living objects or for unique strings. But if properly used it can drastically reduce memory usage. What's really cool - string intern pool is shared across app domains (so multiple web-sites running in the same app pool will share all interned strings)
As we already shortly discussed here #1482 (comment) latest intern related refactoring b1500ea
is not very useful. Almost all
string.Intern
calls were replaced withstring.IsIntern
- and for most cases (likeCollection.Role
or entity aliases) those strings are internally calculated inside NHibernate framework. It's very unlikely that such strings will be interned by user code - so those calls affect performance with no added value.So it's my attempt to implement this feature properly. I think interning should be configurable depending on usage scenario. For instance if it's simple app with single database - there's no need to intern unique session factory metadata like
Collection.Role
. But it might be very useful if multiple session factories/app domains are used.I see configuration levels for interning in the following way:
Disabled - just to make people happy :). Every feature that might affect performance globally should be optional.
Minimal - Very close to current behaviour (make sure that entity names and class names are interned) except I removed some intern pool checks that I think is not useful (like
string.IsIntern
calls forCollection.Role
)Default - Interning level for single session factory scenario. I think it should be default level.
Currently I added property names and column names for this mode as I believe in most databases many column/property names are duplicated (like Id, Name, ParentId, ChildId...)
Also added Suffixes for EntityLoader.
SessionFactories - Interning level for multiple session factories scenario.
Added Entity Aliases for
EntityLoader
(interning is back forDefaultEntityAliases
but only forEntityLoader
),Collection.Role
,PersistentClass.LoaderName
(it was interned by default, but I'm not sure - is it really something that needs to be interned for single session factory?)AppDomains - Multiple app domains scenario.
Added metadata for
StaticProxyFactory
.It's currently just a draft (I didn't finish investigation about what else should be interned) - let me know what you think:
Other thoughts about how it should be done (configuration or anything else)?
What else should be interned?