CODEWITHSUNDEEP
javamultithreading
Kihats
Kihats

Reputation: 3520

ExecutorService inconsistency

I am testing HashIds for collisions. Here is the dependency:

    <!-- https://mvnrepository.com/artifact/org.hashids/hashids -->
    <dependency>
        <groupId>org.hashids</groupId>
        <artifactId>hashids</artifactId>
        <version>1.0.3</version>
    </dependency>

I am using the following code:

    Hashids hashids = new Hashids("xyz", 6, "0123456789ABCDEFGHJKLMNPQRSTUVWXYZ");

    System.out.println("*******************************************************************");
    System.out.println("Started");

    Set<String> set = new HashSet();

    ExecutorService executor = Executors.newFixedThreadPool(4);

    AtomicLong count = new AtomicLong(0);
    final long max = 10_000_000;
    for (long i = 1; i <= max; i++) {

        executor.execute(() -> {
            set.add(hashids.encode(count.incrementAndGet()));

            // This is just to show me that there is some activity going on
            if (count.get() % 100_000 == 0) {
                System.out.println(count.get() + ": " + new Date());
            }
        });
    }

    // Wait till the executor service tasks are done 
    executor.shutdown();
    while (!executor.isTerminated()) {
        Thread.sleep(1000);
    }

    // Assert that the Set did not receive duplicates
    Assert.assertEquals(max, set.size());

    System.out.println("Ended");
    System.out.println("*******************************************************************");

So, I put an ExecutorService to make it a little bit faster but I am running into problems. Either

  1. The ExecutorService does not complete and it hangs there
  2. The Set contains duplicate values therefore the assertion fails
  3. Using the bare for loop is much faster than when using the ExecutorService

What could be wrong with this piece of code?

Upvotes: 2

Views: 213

Answers (2)

eckes
eckes

Reputation: 10423

Queuing a lot of relative fast tasks might be a lot of overhead. You can instead only queue 4 tasks, which loop over a partition of the whole numbers. This will be much faster due to the reduced overhead of queueing and possibly code locality (caches). Besides it avoids concurrent access to counters and collections.

Upvotes: 0

Dai
Dai

Reputation: 407

  1. You need call executor.awaitTermination right after executor.shutdown() ;
  2. Your Set is not thread safe, try wrap your set with Collections.synchronizedSet(set), or create set based on ConcurrentHashMap.newKeySet(), see this discussion about thread safe set.

Upvotes: 3

Related Questions