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

Only parse persistence.xml at build time #31

Closed
wants to merge 5 commits into from

Conversation

stuartwdouglas
Copy link
Member

@stuartwdouglas stuartwdouglas commented Sep 24, 2018

This serializes the persistence.xml to bytecode at build time, which removes the need to parse persistence.xml on startup.

The serialized bytecode looks like this: https://gist.github.com/stuartwdouglas/7498f8f87d0affd5ba6cb4b9f7f9e4f8

@stuartwdouglas
Copy link
Member Author

When running in JVM mode this seems to drop the non-heap memory usage by around 3mb, which should be due to the XML parsers not being loaded.

@emmanuelbernard
Copy link
Member

Way cool! I'll check that out when I'm back spending a few hours on Protean. I'd like to add some comments so that non @stuartwdouglas humans also understand what's going on.

The only thing that does make me a bit itchy (and will make @Sanne too) is that now Hibernate Protean cannot start without being started by Shamrock. I think it's a fine choice in the end but better review it.

@stuartwdouglas
Copy link
Member Author

I have extended this to also provide a Scanner implementation that removes the need to re-scan everything and parse classes with javassist.

After the changes in JVM mode with the jpa-strict example I see ~150ms startup time improvement, ~7mb non-heap memory improvement, and ~2mb heap improvement.

@protean-bot
Copy link
Contributor

You could still bootstrap it manually, by just creating a new class with a static init method that calls the two required methods, however I don't think that this is actually an issue.

If we go past the PoC phase I would argue that all this code should not be a standalone project, but should be tightly integrated into whatever becomes the protean framework architecture. I think this has two big advantages:

  • The integration code will be optimised for Protean, and won't contain anything not needed by the product
  • Spring Boot can't just take it an use it

@stuartwdouglas
Copy link
Member Author

Oops, I was signed in as the CI bot

@n1hility
Copy link
Member

Way cool! I'll check that out when I'm back spending a few hours on Protean. I'd like to add some comments so that non @stuartwdouglas humans also understand what's going on.

The only thing that does make me a bit itchy (and will make @Sanne too) is that now Hibernate Protean cannot start without being started by Shamrock. I think it's a fine choice in the end but better review it.

IMO it makes sense to have the integration code that shamrock uses tied to shamrock. Shamrock is flexible enough that it covers the use-case of standalone hibernate, so there isn't much a benefit to supporting non-shamrock substrate integration other than making it easier for competitors to pick it up.

@Sanne
Copy link
Member

Sanne commented Oct 1, 2018

Ok just found the related changes and started a room in zulip:

I have no problems to depend on Shamrock but I'm not understanding how this works :)

In particular, I already did NOT need to parse the XML configuration files in native; we were pre-parsing them during metadata analysis, and not even including the resources in the binaries.

So this looks like a step backwards to me as now we're actually serializing a representation of these resources, which are not necessary.

You mention a memory improvement though, so I'm likely missing something of how this stuff works?

@Sanne
Copy link
Member

Sanne commented Oct 1, 2018

Had a chat with @stuartwdouglas and he reminded me of the benefit of not parsing these in JVM mode either. So maybe there's some unnecessary code which gets included in the native thing as well, but assuming we can trust DCE I guess I shouldn't nitpick about that.

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.

6 participants