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

fix(core) promise_id should be 32bit instead of 64bit #9720

Closed
wants to merge 1 commit into from
Closed

fix(core) promise_id should be 32bit instead of 64bit #9720

wants to merge 1 commit into from

Conversation

inteon
Copy link
Contributor

@inteon inteon commented Mar 7, 2021

Extracted this inconsistency fix from #9457.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@inteon I'm not sure this change is preferable; running out of 32bit promise ids could be possible after few hours of server running. What's the reasoning behind this change?

@inteon
Copy link
Contributor Author

inteon commented Mar 9, 2021

In MinimalOp, i32 is used for promise_id and in json_ops u64 is used.
In javascript the promise_id is only converted to a big int BigInt(promiseId) before it is passed to rust.
I'm trying to make all these promise_ids the same length.
But, like you said, maybe 64 bits should be used?
I would then change i32 to u64, and use BigInt in js for the counter too.

@bnoordhuis
Copy link
Contributor

Conversion to BigInt isn't necessary and introduces a small performance penalty.

2^32 is too small but JS numbers are exact up to 2^53 and that's still unfathomably large (good for the next 285 years at a rate of 1 million promises/sec.)

@inteon
Copy link
Contributor Author

inteon commented Mar 10, 2021

Ok, I get it now; in javascript a Number is used, which is more performant than an BigInt and can represent integer values up to ±2^53 − 1 exactly (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Numbers_and_dates).

In order to send these 53 useful bits, setUint32 cannot be used.
Instead the Number is converted to a BigInt so it can be used in setBigUint64.

To summarize:

  • in rust a promise_id is an u64
  • in javascript a promise_id is a Number
    Because of js, the usable promise_id range is [0, 2^53 - 1], so 11 js <-> rs transferred bits will be left unused.

I will change the MinimalOps behavior to match this, and this PR is no longer relevant.

@inteon inteon closed this Mar 10, 2021
@inteon inteon changed the title fix(core) promise_id is 32bit instead of 64bit fix(core) promise_id should be 32bit instead of 64bit Mar 10, 2021
@inteon inteon deleted the move_minimal_ops_to_core_step2 branch March 11, 2021 16:38
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.

3 participants