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

More performance updates #2034

Merged
merged 18 commits into from
Nov 7, 2019
Merged

More performance updates #2034

merged 18 commits into from
Nov 7, 2019

Conversation

ssalinas
Copy link
Member

@ssalinas ssalinas commented Nov 4, 2019

  • remove all parallelStream calls and repalce with a managed executor
  • reduce default concurrency everywhere
  • bail out of offer checks and decline if offer processing is too delayed
  • add a good enough threshold on offer scores to skip processing additional checks
  • sql history purger doesn't need to hold the lock since it doesn't touch zk

more to come soon....

@ssalinas
Copy link
Member Author

ssalinas commented Nov 5, 2019

Next round of updates:

  • bump the status update concurrency back up a slight bit since we spend much of the time waiting for locks, not in cpu-intensive things
  • load leader cache in parallel. This is 99% of our startup time right now once subscribed
  • update the offer loop to wait for locks in parallel, but run offer checking in serial. Have to test this more but it should make the overall offer loop much faster
  • use a timeout when acquiring the offers lock for net-new offers. If we don't get it in time, cache these offers instead. This will let the offer loop run as fast as it can without backing up, and in general process larger batches of offers, with more possibility for finding a match
  • bump the UI notification back to 100 late requests instead of 20

@ssalinas
Copy link
Member Author

ssalinas commented Nov 5, 2019

latest additions:

  • don't let the persister hold the lock for more than 10s at a time. We can always catch up on persisting later

@@ -73,8 +72,7 @@ public SnsWebhookManager(@Singularity ObjectMapper objectMapper,
this.snsClient = AmazonSNSClientBuilder.defaultClient();
}
this.webhookManager = webhookManager;
this.publishSemaphore = AsyncSemaphore.newBuilder(configuration::getMaxConcurrentWebhooks, executorServiceFactory.get("webhook-publish-semaphore", 1)).build();
this.publishExecutor = managedCachedThreadPoolFactory.get("webhook-publish");
this.publishExecutor = managedCachedThreadPoolFactory.get("webhook-publish", configuration.getMaxConcurrentWebhooks());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a note here to double check our naming here, for both managedCachedThreadPoolFactory and maxConcurrentWebhooks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did the rename for these. Going to leave the config var the same though for easier backwards compatibility. It is essentially still controlling the same thing, we are just being limited by the size of a fixed thread pool vs the async semaphore now. A future update could be to consolidate all the thread pool size configs in one spot

statePoller.wake();
scheduler.start();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small change, but I'm curious: why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we call the state poller first we get instant feedback in the UI about who is leader versus having to wait until after startup is already finished to know which host gained leadership

@baconmania
Copy link
Contributor

🚢

@ssalinas ssalinas merged commit 6232ca6 into master Nov 7, 2019
@ssalinas ssalinas deleted the more_performance branch November 7, 2019 13:47
@ssalinas ssalinas added this to the 1.1.0 milestone Nov 12, 2019
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.

2 participants