Skip to content

Avoid range checks in nan-boxing#4251

Merged
hansl merged 1 commit intomainfrom
nan-boxing-tag
May 17, 2025
Merged

Avoid range checks in nan-boxing#4251
hansl merged 1 commit intomainfrom
nan-boxing-tag

Conversation

@raskad
Copy link
Member

@raskad raskad commented May 17, 2025

While profiling, I noticed, that especially in the NavierStokes benchmark, the NanBoxedValue::as_variant function was taking a lot of runtime. I think the range checks caused this. I refactored the nan-boxing a bit, so that the value kind is always encoded in bits 48-50. Because this only leaves space for 7 kinds, I encoded null and undefined into one kind.
Currently we use bit 51 for kind encoding, but as far as I understood the sources I looked at, if bit 51 is 0, the NaN value is a signaling NaN. But to be honest I have not found any real indicators, that using the bit, as long as you do not pass a signaling NaN back into a floating point algorithm, causes big issues.

When running the benchmarks, for most this change has no real impact. But in NavierStokes I get a slight performance increase (~5%).

@raskad raskad requested a review from a team May 17, 2025 02:02
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,254 50,254 0
Passed 46,898 46,898 0
Ignored 1,634 1,634 0
Failed 1,722 1,722 0
Panics 0 0 0
Conformance 93.32% 93.32% 0.00%

Copy link
Contributor

@hansl hansl left a comment

Choose a reason for hiding this comment

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

LGTM. You're right that signaling NaNs aren't an issue. I thought I had that written somewhere (and the rationale) but it's not in the comments. Mmmh.

@hansl hansl added this pull request to the merge queue May 17, 2025
Merged via the queue into main with commit 14b88c9 May 17, 2025
14 checks passed
@hansl hansl deleted the nan-boxing-tag branch May 17, 2025 04:16
@andreievg
Copy link

andreievg commented Jun 6, 2025

@raskad, we are running rust build for android as .so, getting a panic with this:

2025-06-06 09:27:39.053110765 [ERROR] <log_panics:130>:thread '<unnamed>' panicked at 'assertion `left == right` failed: Pointer is not 4-bits aligned or over 51-bits.
  left: 499905826256
 right: 12970367426732854736': /.cargo/git/checkouts/boa-126d820dff07bf57/6f03c21/core/engine/src/value/inner/nan_boxed.rs:379
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: <unknown>
  18: <unknown>
  19: <unknown>
  20: <unknown>
  21: <unknown>
  22: <unknown>
  23: <unknown>
  24: <unknown>
  25: <unknown>
  26: <unknown>
  27: <unknown>
  28: <unknown>
  29: <unknown>
  30: <unknown>
  31: <unknown>
  32: <unknown>
  33: _ZL15__pthread_startPv
  34: __start_thread

Will log an issue

@raskad
Copy link
Member Author

raskad commented Jun 6, 2025

@raskad, we are running rust build for android as .so, getting a panic with this:

2025-06-06 09:27:39.053110765 [ERROR] <log_panics:130>:thread '<unnamed>' panicked at 'assertion `left == right` failed: Pointer is not 4-bits aligned or over 51-bits.
  left: 499905826256
 right: 12970367426732854736': /.cargo/git/checkouts/boa-126d820dff07bf57/6f03c21/core/engine/src/value/inner/nan_boxed.rs:379
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: <unknown>
  16: <unknown>
  17: <unknown>
  18: <unknown>
  19: <unknown>
  20: <unknown>
  21: <unknown>
  22: <unknown>
  23: <unknown>
  24: <unknown>
  25: <unknown>
  26: <unknown>
  27: <unknown>
  28: <unknown>
  29: <unknown>
  30: <unknown>
  31: <unknown>
  32: <unknown>
  33: _ZL15__pthread_startPv
  34: __start_thread

Will log an issue

__start_thread

Hi @andreievg that looks bad, please open an issue so we can track it. I already have some questions about your setup etc. The commit you are using (6f03c21) is before this PR landed, so could you checkout the latest commit on main and try the same with that? The same asserts still exist and this Pr should have not really changed anything, but better to check it.
Also the is a runtime assert, so you build succeeded, but it failed during the runtime, correct? Can you reproduce it and if possible post the code that you run to produce the error?
Also specific information about your setup, architecture etc. would be really usefull.

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