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

Replace ThreadLocal cache with ThreadIdCache #11504

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Mar 11, 2024

Use a cache class that will work better with virtual threads.

This generalized the "Pool-lite" algorithm introduced in PR #11498 to provide a cache that can be used to fix #10568

gregw added 2 commits March 11, 2024 15:03
Use a cache class that will work better with virtual threads
Use a cache class that will work better with virtual threads
@gregw
Copy link
Contributor Author

gregw commented Mar 11, 2024

@lorban @sbordet I have now also replaced usages of ThreadLocalRandom with ThreadIdCache.Random, so we will not leak Random instances when using virtual threads.

For non-cache usages of ThreadLocal, we should look at changing the frequent pattern:

    Object oldValue = threadLocal.get();
    try
    {
        threadLocal.set(newValue);
        // ...
    } 
    finally
    {
        threadLocal.set(oldValue);
    }

to be replaced with the pattern (probably with help of a utility method):

    Object oldValue = threadLocal.get();
    try
    {
        threadLocal.set(newValue);
        // ...
    } 
    finally
    {
        if (oldValue == null)
            threadLocal.remove();
       else
            threadLocal.set(oldValue);
    }

or maybe even (@lorban is this worth the extra if?)

    Object oldValue = threadLocal.get();
    try
    {
        threadLocal.set(newValue);
        // ...
    } 
    finally
    {
        if (oldValue == null && VirtualThreads.isVirtualThread())
            threadLocal.remove();
       else
            threadLocal.set(oldValue);
    }

thoughts?

@gregw gregw marked this pull request as ready for review March 11, 2024 17:41
@gregw gregw requested review from sbordet and lorban March 12, 2024 14:36
{
if (capacity >= 0)
return capacity;
return 2 * ProcessorUtils.availableProcessors();
Copy link
Contributor

Choose a reason for hiding this comment

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

Prooooobably just the number of cores is right!

@gregw gregw merged commit 2a543ac into experiment/jetty-12.0.x/reservedThreadExecutor Mar 12, 2024
1 of 3 checks passed
@gregw
Copy link
Contributor Author

gregw commented Mar 12, 2024

I have merged this to #11498, where is can be further reviewed.

@gregw gregw deleted the fix/jetty-12.0.x/10568/NoThreadLocalCache branch March 12, 2024 16:25
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