Avoid UUID.randomUUID() in file system related startup code by geoand · Pull Request #5450 · eclipse-vertx/vert.x

geoand · 2025-01-16T11:39:24Z

Motivation:

This is done because bootstrapping the plumbing
needed by the JDK to produce a UUID value
is expensive, it thus doesn't make sense to
pay this cost when the property isn't actually
needed

Explain here the context, and why you're making that change, what is the problem you're trying to solve.

We are making an effort in Quarkus to improve startup time even further by eliminating various bottlenecks across the board.
The first call to UUID.randomUUID() is definitely heavy (as shown in the following flamegraph) and if we can avoid it a startup code (as we have in the development branch of Quarkus), it would be nice.

P.S. Ideally we would like to have this in Vert.x 4 as well.

franz1981 · 2025-01-16T12:45:44Z

vertx-core/src/main/java/io/vertx/core/file/impl/FileCache.java

    // the cacheDir will be suffixed a unique id to avoid eavesdropping from other processes/users
    // also this ensures that if process A deletes cacheDir, it won't affect process B
-    String cacheDirName = fileCacheDir + "-" + UUID.randomUUID();
+    String cacheDirName = fileCacheDir + "-" + System.nanoTime();


nanoTime is not absolute - it's relative to the process. Meaning that another application starting can have it again, without any need to be simultaneous - is it what you expect?

nanoTime is not absolute - it's relative to the process

Correct. But we do think it can be problematic, I'm happy to use Random.getRandom() or System.currentTimeMillis()

if we use Math.random() it get better - but is still not granted to be unique - because it still uses System::nanoTime and Random per se doesn't guarantee uniqueness across processes (try printing new Random(42).nextInt() running it twice with 2 diff processes...)

I am pretty sure we are not looking for that such strong of a guarantee here, but I'll let the maintainers be the judge of that

How about an optimistic attempt? Something like (simplifying):

for(;;) { try { String cacheDirName = fileCacheDir + "-" + System.nanoTime(); Files.createDirectories(cacheDirName); break; } catch(FileAlreadyExistException ignore) { } }

In fact, you could use a random instead of System.nanoTime, I think it would be faster

@franz1981 is Random.nextLong() faster than System.nanoTime()?

nope - or better - usually nanoTime (if not on the cloud with unreliable time sources) uses a thing called rdts which is as cheap as reading a memory area

tsegismont · 2025-01-16T17:04:28Z

vertx-core/src/main/java/io/vertx/core/impl/deployment/DefaultDeploymentManager.java


  private String generateDeploymentID() {
-    return UUID.randomUUID().toString();
+    return Long.valueOf(System.nanoTime()).toString();


This needs to be globally unique when running in clustered mode with HA enabled

So perhaps something like:

if (vertx.isClustered() && vertx.haManager()!=null) { return UUID.randomUUID().toString(); } // Use a counter?

It's pretty common to deploy verticles concurrently. Even when Vert.x is not clustered, the returned value should be unique.

I have updated it to use Random, is that what you meant?

I meant incrementing an AtomicLong counter instead of using a random value (uniqueness is guaranteed and it shouldn't change the perf results you got)

Got it, fixed

vietj · 2025-01-16T17:39:34Z

what seems to take time is the initialization of SecureRandom.getDfaultPrng due to loading providers, I think w ecould generate a faster UUID by using a given provider

geoand · 2025-01-16T17:43:43Z

what seems to take time is the initialization of SecureRandom.getDfaultPrng due to loading providers, I think w ecould generate a faster UUID by using a given provider

But those are not public APIs, no?

vietj · 2025-01-16T17:46:50Z

I think we should have a way to specify the exact cache dir (e.g. FileSystemOptions#exactFileCacheDir), when none is provided then it uses UUID. Quarkus would specify it in VertxOptions. This would easily be back-ported

geoand · 2025-01-16T17:53:40Z

Sure, that would make sense for us too

geoand · 2025-01-17T07:20:28Z

I have updated the PR per suggestions

geoand · 2025-01-21T11:58:19Z

Is there anything else you would like me to do for this one?

tsegismont · 2025-01-24T10:29:43Z

I think we should have a way to specify the exact cache dir (e.g. FileSystemOptions#exactFileCacheDir), when none is provided then it uses UUID. Quarkus would specify it in VertxOptions. This would easily be back-ported

For usability, it seems to me adding a boolean to the options would be enough (it's what's computed in the end to determine if a UUID should be added to the path).

But it's a matter of taste so I'm fine with keeping an extra dir option if you choose so @vietj

geoand · 2025-01-30T06:35:37Z

Is there anything more you want me to do with this one?

tsegismont

LGTM, thank you @geoand

tsegismont · 2025-01-30T09:37:10Z

@vietj PTAL

geoand · 2025-01-30T09:38:20Z

🙏🏽

vertx-core/src/main/java/io/vertx/core/file/FileSystemOptions.java

vietj · 2025-01-31T08:24:57Z

vertx-core/src/main/java/io/vertx/core/impl/deployment/DefaultDeploymentManager.java


  public static final Logger log = LoggerFactory.getLogger(DefaultDeploymentManager.class);

+  private static final AtomicLong nextId = new AtomicLong();


the idea of cache dir is to avoid that, and keep the same behaviour we have, so please no.

So what do you propose? This was added as a response to #5450 (comment)

I believe you confused two changes @vietj : the cache dir that used a random UUID, and the verticle id generator

sorry, can we have the verticle id generator in another PR then ?

I'd like to keep distinct PR for the changelog

Distinct PR or commit?

vietj

can you add a test with a real vertx instance and check those cases

a missing dir is created
an existing dir is reused
an error is thrown when a non dir file exist already

geoand · 2025-02-04T13:58:23Z

Sure, I'll do that when I'm back from JFokus

geoand · 2025-02-06T07:52:34Z

Aren't those cases already covered by the tests for FileResolver?

vietj · 2025-02-06T15:25:02Z

Aren't those cases already covered by the tests for FileResolver?

good question, I don't know :-)

geoand · 2025-02-06T16:00:03Z

That's what I understand from looking at FileResolverTestBase

vietj · 2025-02-10T08:27:23Z

vertx-core/src/test/java/io/vertx/tests/file/FileResolverTestBase.java

+  public void testGetTheExactCacheDirWithoutHacks() {
+    String cacheDir = new FileResolverImpl(new FileSystemOptions().setExactFileCacheDir(cacheBaseDir + "-exact")).cacheDir();
+    if (cacheDir != null) {
+      System.out.println(cacheDir);


vietj · 2025-02-10T08:28:30Z

vertx-core/src/test/java/io/vertx/tests/file/FileResolverTestBase.java

  }
+
+  @Test
+  public void testGetTheExactCacheDirWithoutHacks() {


this test should be moved to FileCacheTest instead, FileResolverTestBase tests the behaviour of resolver implementations

vietj

We need tests that assesses the behaviour of creating a vertx instance when

the cache dir already exists and is a directory (I guess it reuses the directory)
the cache dir does not exist (it should create the missing directory)
the cache dir string is not a valid value
the cache dir does not exists and cannot be created, e.g. the parent path points to a file
the cache dir exists but is not a directory, e.g. it is a file

geoand · 2025-02-13T10:37:34Z

I added all but 3 as I didn't find a way to add a invalid name

pmlopes · 2025-02-14T13:53:20Z

vertx-core/src/main/java/io/vertx/core/file/impl/FileCache.java

    // also this ensures that if process A deletes cacheDir, it won't affect process B
-    String cacheDirName = fileCacheDir + "-" + UUID.randomUUID();
-    File cacheDir = new File(cacheDirName);
+    File cacheDir = isEffectiveValue ? new File(fileCacheDir) : new File(fileCacheDir + "-" + UUID.randomUUID());


Doesn't this change, break the comment above? when isEffectiveValue is true, 2 vert.x instances will interfere with each other's cache. While this is probably ok for the same application, if 2 applications differ, then it could cause invalid states.

One example is (regardless if the 2 applications are the same or not) The moment the 1st terminates, it deletes the cache and would also mean it was deleted for the second, causing inconsistencies and errors.

I think it is fine because effective value is part of the options and users have to explicitly enable it, so they control it and have to care about it

geoand · 2026-01-09T16:15:24Z

Is there any chance we can move this forward? @gsmet also independently ran into it

geoand · 2026-02-05T14:12:30Z

I would really appreciate some input on this

This is done because bootstrapping the plumbing needed by the JDK to produce a UUID value is expensive, it thus doesn't make sense to pay this cost when the property isn't actually needed

geoand · 2026-02-06T18:35:14Z

If this is approved, could it be backported to 4?

vietj · 2026-02-07T07:21:41Z

@geoand I would jsut like before to change the configuration option, instead of passing a different path exactFileCacheDir, I would prefer a boolean that configurations whether to use the existing path fully or as a directory.

geoand · 2026-02-07T07:43:51Z

@vietj Thanks. Can you elaborate a little more on what exactly you would like to see, because I don't fully underastand your proposal

geoand mentioned this pull request Jan 16, 2025

Remove the use of UUID in startup code quarkusio/quarkus#45610

Merged

geoand force-pushed the remove-uuid branch from cd16a25 to f190b2e Compare January 16, 2025 11:41

franz1981 reviewed Jan 16, 2025

View reviewed changes

tsegismont reviewed Jan 16, 2025

View reviewed changes

geoand force-pushed the remove-uuid branch from f190b2e to f1a57fd Compare January 16, 2025 17:43

geoand force-pushed the remove-uuid branch 2 times, most recently from b28445c to 7c9fc8f Compare January 17, 2025 07:19

geoand force-pushed the remove-uuid branch from 7c9fc8f to 9c28bb3 Compare January 24, 2025 10:31

tsegismont requested a review from vietj January 30, 2025 09:36

tsegismont approved these changes Jan 30, 2025

View reviewed changes

vietj reviewed Jan 31, 2025

View reviewed changes

vertx-core/src/main/java/io/vertx/core/file/FileSystemOptions.java Outdated Show resolved Hide resolved

vietj reviewed Jan 31, 2025

View reviewed changes

geoand force-pushed the remove-uuid branch 2 times, most recently from 8ba99cf to c749aaa Compare January 31, 2025 15:55

geoand mentioned this pull request Jan 31, 2025

Avoid UUID.randomUUID() in Verticle deployment startup code #5469

Merged

geoand changed the title ~~Avoid UUID.randomUUID() in startup code~~ Avoid UUID.randomUUID() in file system related startup code Jan 31, 2025

vietj requested changes Feb 4, 2025

View reviewed changes

vietj added this to the 5.0.0 milestone Feb 10, 2025

vietj reviewed Feb 10, 2025

View reviewed changes

vietj requested changes Feb 10, 2025

View reviewed changes

geoand force-pushed the remove-uuid branch from c749aaa to 3444ddf Compare February 13, 2025 10:35

geoand force-pushed the remove-uuid branch from 3444ddf to 7e04c3a Compare February 13, 2025 15:41

pmlopes reviewed Feb 14, 2025

View reviewed changes

vietj force-pushed the remove-uuid branch from 7e04c3a to 522cfe7 Compare May 12, 2025 13:34

vietj modified the milestones: 5.0.0, 5.1.0 May 14, 2025

geoand force-pushed the remove-uuid branch 2 times, most recently from c94173c to 5a95731 Compare January 23, 2026 06:43

geoand requested a review from vietj January 23, 2026 06:44

geoand force-pushed the remove-uuid branch from 5a95731 to a7b1dc2 Compare January 23, 2026 06:46

geoand force-pushed the remove-uuid branch from a7b1dc2 to 9b14e58 Compare February 3, 2026 17:30

Avoid UUID.randomUUID() in file system related startup code

7718825

This is done because bootstrapping the plumbing needed by the JDK to produce a UUID value is expensive, it thus doesn't make sense to pay this cost when the property isn't actually needed

geoand force-pushed the remove-uuid branch from 9b14e58 to 7718825 Compare February 5, 2026 14:32


		public static final Logger log = LoggerFactory.getLogger(DefaultDeploymentManager.class);

		private static final AtomicLong nextId = new AtomicLong();

Conversation

geoand commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

franz1981 Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsegismont Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vietj commented Jan 16, 2025

Uh oh!

geoand commented Jan 16, 2025

Uh oh!

vietj commented Jan 16, 2025

Uh oh!

geoand commented Jan 16, 2025

Uh oh!

geoand commented Jan 17, 2025

Uh oh!

geoand commented Jan 21, 2025

Uh oh!

tsegismont commented Jan 24, 2025

Uh oh!

geoand commented Jan 30, 2025

Uh oh!

tsegismont left a comment

Choose a reason for hiding this comment

Uh oh!

tsegismont commented Jan 30, 2025

Uh oh!

geoand commented Jan 30, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoand Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoand commented Jan 16, 2025 •

edited

Loading

franz1981 Jan 16, 2025 •

edited

Loading

tsegismont Jan 16, 2025 •

edited

Loading

geoand Jan 31, 2025 •

edited

Loading

geoand commented Feb 13, 2025 •

edited

Loading