Skip to content
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

HSEARCH-2203 Reduce memory consumption of EntityInfoImpl objects. #1053

Closed
wants to merge 3 commits into from

Conversation

golovnin
Copy link
Contributor

@golovnin golovnin commented Apr 4, 2016

indexesOfThis.add( i );
// Use THIS as placeholder. It will be replaced
// when we populate the EntityInfo with the real entity.
projections[i] = ProjectionConstants.THIS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than reusing the String from ProjectionConstants.THIS, I was considering using a marker object.

Would you prefer to create a (static final) singleton having just a nice toString() implementation so it doesn't get too confusing during debug?
You would still use reference equality to check for the marker, but it makes me more comfortable than doing it on a string, especially to not reuse the constant.

@Sanne
Copy link
Member

Sanne commented Apr 5, 2016

Thanks @golovnin , looks great! I made some minor comments, not sure if we should change them. Up to you, so I'll keep this open for some more days.

Seems like you're pushing performance requirements hard? Would be nice to hear more about what you're doing and aiming at.

@Sanne Sanne self-assigned this Apr 5, 2016
@gunnarmorling
Copy link
Member

So does this loose the ability "this" more than once? I doubt anyone would be doing it, but it was allowed before, so we'd at least need an entry in the migration notes.

@Sanne
Copy link
Member

Sanne commented Apr 5, 2016

So does this loose the ability "this" more than once?

Why do you say that? I didn't notice.

@gunnarmorling
Copy link
Member

Yeah, seems alright. The removal of getIndexesOfThis() made me wonder, but it's handled as part of that array now.

Uses explicit placeholder object instead of ProjectionConstants.THIS.
Stores the entity instance in a field of EntityInfoImpl to avoid
the iteration through the projections with instanceof attempts.
@golovnin
Copy link
Contributor Author

golovnin commented Apr 5, 2016

@Sanne I changed the code as you suggested. I hope that the toString()-method of EntityInfo.ENTITY_PLACEHOLDER is nice enough. :-)

@Sanne
Copy link
Member

Sanne commented Apr 6, 2016

Thanks! I integrated it.

@Sanne Sanne closed this Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants