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

prevent arithmetic overflow and reduce allocations in time conversions #929

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

magicprinc
Copy link
Contributor

@magicprinc magicprinc commented Nov 20, 2023

See java.lang.Math#multiplyExact(long, long)

ArithmeticException can be thrown

and temporary Duration objects are created

@magicprinc
Copy link
Contributor Author

magicprinc commented Nov 20, 2023

The final version:

  • doesn't create temporary Duration object
  • doesn't suffer from ArithmeticException (long overflow)
  • supports more ChronoUnit than before (e.g. MONTHS)

@Ladicek
Copy link
Contributor

Ladicek commented Nov 21, 2023

Good point, I didn't realize that. I generally think that overflow would be rare, so we could just wrap the Duration.toMillis() call in a try/catch and return Long.MAX_VALUE, but if we can avoid allocating, that's even better :-)

@Ladicek Ladicek force-pushed the bugfix/duration_overflow branch from dcacacc to d2fabf2 Compare November 21, 2023 10:16
@Ladicek
Copy link
Contributor

Ladicek commented Nov 21, 2023

I removed the Thread.yield() call, because I haven't seen conclusive evidence that this is actually helpful. Do you have any benchmarks that motivated this change?

@Ladicek Ladicek added this to the 6.3.0 milestone Nov 21, 2023
@Ladicek Ladicek changed the title bugfix: Duration.of and toMillis can throw ArithmeticException prevent arithmetic overflow and reduce allocations in time conversions Nov 21, 2023
@Ladicek Ladicek merged commit 6066596 into smallrye:main Nov 21, 2023
10 checks passed
@magicprinc
Copy link
Contributor Author

About Thread.yield():

I have made tests now, and it should be Thread.sleep(0) actually.

sleep(0) works and it allows thread interruption

sleep and onSpinWait are very strange methods in this aspect :-)

@Ladicek
Copy link
Contributor

Ladicek commented Nov 21, 2023

Thread.onSpinWait() would be a mistake here, the ThreadSleepDelay is not busy waiting / spinning.

If I understand correctly, you attempted to play nice in the cooperative multi-threading world of virtual threads, but I believe the retry strategy is not the right place for that. If the guarded logic is heavy on computations and monopolizes the carrier thread, it is its job to play nice.

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