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

Save complex OS-level I/O to know the CPU topology #216

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

franz1981
Copy link
Collaborator

This is related smallrye/smallrye-common#370 since smallrye-common exposes CacheInfo as a public API and dropping it can impact existing users - while doing it here, won't impact anyone.

The benefits are quarkusio/quarkus#45436

threadStatusOffset = unsafe.arrayBaseOffset(long[].class) + pad;
queueSizeOffset = unsafe.arrayBaseOffset(long[].class) + pad * 2;
} else {
unsharedTaskNodesSize = numUnsharedObjects;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dmlloyd we have no coverage for this unless we run our test suite with -XX:ActiveProcessorCount=1
so please take a look if I did something wrong

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 8, 2025

This PR will conflict with #215 so we should coordinate somehow.

@franz1981
Copy link
Collaborator Author

since is a small change @dmlloyd here, feel free to cp into your branch without authoring me ❤️

@Sanne
Copy link
Contributor

Sanne commented Jan 10, 2025

Could we take it a step further?

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 13, 2025

The library exists to be used. I'd rather change the logic of the library so that it assumes a reasonable cache line size (perhaps based on something that is faster to ascertain, or just with a sane default) and allows a system property override (with values of, for example, detect, 256, 64, etc.) for edge cases where the default is not suitable.

@geoand
Copy link
Contributor

geoand commented Jan 14, 2025

I vote for having a sane default and having people opt-in to more complex handling as:

  • The default is probably fine for almost all cases we are trying to address
  • Trying to figure out the actual cache line info is one of the few things whose elimination has an undeniably measurable impact (especially on Mac)

@franz1981
Copy link
Collaborator Author

@dmlloyd do you think we can make this one to slip in first? So we can take our time into verifying that Unsafe-less we don't regress in runtime perf too - not just RSS? 🙏

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 20, 2025

I think we should fix this only in smallrye-common. Otherwise, as soon as someone else uses those functions, we'll pay the cost anyway, and we'll end up with two different analyses of cache lines.

@geoand
Copy link
Contributor

geoand commented Jan 20, 2025

smallrye/smallrye-common#389 is a simply way to do that

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.

4 participants