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

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

@@ -45,7 +45,7 @@ static File setupCacheDir(String fileCacheDir) {

    // 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

@@ -36,7 +36,7 @@ public DefaultDeploymentManager(VertxImpl vertx) {
  }

  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

@@ -27,6 +28,8 @@ public class DefaultDeploymentManager implements DeploymentManager {

  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

@@ -486,4 +488,16 @@ public void testGetTheCacheDirWithoutHacks() {
      }
    }
  }
+
+  @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

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

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

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

    // ensure that the argument doesn't end with separator
    if (fileCacheDir.endsWith(File.separator)) {
      fileCacheDir = fileCacheDir.substring(0, fileCacheDir.length() - File.separator.length());
    }

    // 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();
-    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.

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

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

7e04c3a

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 3444ddf to 7e04c3a Compare February 13, 2025 15:41

pmlopes reviewed Feb 14, 2025

View reviewed changes

		@@ -27,6 +28,8 @@ public class DefaultDeploymentManager implements DeploymentManager {

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

		private static final AtomicLong nextId = new AtomicLong();

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

Are you sure you want to change the base?

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

Conversation

geoand commented Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsegismont Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vietj commented Jan 16, 2025

geoand commented Jan 16, 2025

vietj commented Jan 16, 2025

geoand commented Jan 16, 2025

geoand commented Jan 17, 2025

geoand commented Jan 21, 2025

tsegismont commented Jan 24, 2025

geoand commented Jan 30, 2025

tsegismont left a comment

Choose a reason for hiding this comment

tsegismont commented Jan 30, 2025

geoand commented Jan 30, 2025

Choose a reason for hiding this comment

geoand Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vietj left a comment

Choose a reason for hiding this comment

geoand commented Feb 4, 2025

geoand commented Feb 6, 2025

vietj commented Feb 6, 2025

geoand commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vietj left a comment

Choose a reason for hiding this comment

geoand commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

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