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

ArcContainerImpl could skip SecureRandom initialization #4490

Closed
Sanne opened this issue Oct 10, 2019 · 4 comments · Fixed by #4676
Closed

ArcContainerImpl could skip SecureRandom initialization #4490

Sanne opened this issue Oct 10, 2019 · 4 comments · Fixed by #4676
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Milestone

Comments

@Sanne
Copy link
Member

Sanne commented Oct 10, 2019

Description
The 'ArcContainerImpl' is eagerly initializing an uuid field, which requires a Secure random call; according to flamegraphs including native time, this is taking 30% of the time it takes to boot Arc so it would be nice to skip it.

Implementation ideas
The uuid field seems only necessary for toString(). I think it could be lazily initialized, I'm not sure what expectation there is in terms of having the same uuid (or not) of the same app when it's build as native.

I suppose it should mimick the JVM semantics, which implies lazy initialization would actually be better.

@Sanne Sanne added kind/enhancement New feature or request area/arc Issue related to ARC (dependency injection) labels Oct 10, 2019
@mkouba
Copy link
Contributor

mkouba commented Oct 14, 2019

this is taking 30% of the time it takes to boot Arc so it would be nice to skip it.

Interesting. How much time does this 30% represent?

The uuid field seems only necessary for toString().

Yes, it is. Although it could be accessible through the ArcContainer interface in the future. The idea is to have a unique identifier of a container instance. On the other hand, we don't support running parallel container instances ATM and so it's mostly useful for debugging. In theory, we could use a simple static counter as well.

@mkouba
Copy link
Contributor

mkouba commented Oct 18, 2019

@Sanne pls provide more info...

@Sanne
Copy link
Member Author

Sanne commented Oct 19, 2019

hi @mkouba - to provide more details I'll need to run those metrics again; it takes me quite some time and won't be able to do it this week.

Of course the win isn't very high in absoulte terms, as Arc boots quite fast - we're talking about milliseconds. But considering the spirit of Quarkus if this uuid isn't used I'd just avoid it: it's not only about time but also about including / loading the securerandom related classes - and the system code necessary to support these.

I'd remove this UUID as it seems very simple to do; you could just re-introduce it in case of need?
Otherwise if you think it's needed, lets' move on and close this.

@mkouba
Copy link
Contributor

mkouba commented Oct 20, 2019

@Sanne Ok. No need to run the tests again. I've already fixed this in #4676.

@gsmet gsmet added this to the 0.26.0 milestone Oct 22, 2019
CSTDev pushed a commit to CSTDev/quarkus that referenced this issue Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants