Introduce NonMaxU32 for array index #2131

CYBAI · 2022-06-20T04:11:18Z

It changes the following:

Introduce NonMaxU32 type
Use NonMaxU32 for index property key instead of u32

While looking into the failed built-ins/Reflect/ownKeys/return-on-corresponding-order-large-index.js in test262, I found that the root cause is related to array index for property keys.

The root cause of the failure is, the test uses u32::MAX as key and we're ordering it wrong because we see u32::MAX as a array index instead of an integer index. Based on the spec, array index's range is from 0 <= n < u32::MAX so the max number is exclusive.

We can avoid transforming u32::MAX into property key in some functions like From<T> traits though, it might be forgettable and error-prone. So, I wonder it might be good to introduce the NonMaxU32 type which is similar to NonZeroU32 in std but excluding max instead.

Then, we can use it for the index property key

-    Index(u32),
+    Index(NonMaxU32),

With doing so, we can always guarantee the integer hold by Index property key is valid.

WDYT?

(On the other hand, I'm not sure where is the best place for such types. Please feel free to let me know if I should move it to other places 🙏)

codecov · 2022-06-20T04:20:24Z

Codecov Report

Merging #2131 (f18bf35) into main (d173e08) will decrease coverage by 0.05%.
The diff coverage is 25.75%.

@@            Coverage Diff             @@
##             main    #2131      +/-   ##
==========================================
- Coverage   43.48%   43.43%   -0.06%     
==========================================
  Files         220      221       +1     
  Lines       19983    20009      +26     
==========================================
+ Hits         8689     8690       +1     
- Misses      11294    11319      +25

Impacted Files	Coverage Δ
boa_engine/src/lib.rs	`86.66% <ø> (ø)`
...ine/src/object/internal_methods/integer_indexed.rs	`0.00% <0.00%> (ø)`
boa_engine/src/object/internal_methods/mod.rs	`67.53% <0.00%> (-0.44%)`	⬇️
boa_engine/src/nonmaxu32.rs	`14.81% <14.81%> (ø)`
...oa_engine/src/object/internal_methods/arguments.rs	`30.76% <20.00%> (ø)`
boa_engine/src/object/internal_methods/string.rs	`36.17% <33.33%> (ø)`
boa_engine/src/property/mod.rs	`50.88% <61.53%> (-0.01%)`	⬇️
boa_engine/src/object/internal_methods/array.rs	`76.36% <100.00%> (-1.82%)`	⬇️
boa_engine/src/object/property_map.rs	`20.00% <100.00%> (ø)`
...oa_engine/src/syntax/parser/statement/block/mod.rs	`41.46% <0.00%> (-2.44%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d173e08...f18bf35. Read the comment docs.

Based on the definition in spec, [array index][] is only valid from zero to u32::MAX - 1. It would be helpful to use the NonMaxU32 type to define it as the property key instead so that contributors won't forget to check the validity. [array index]: https://tc39.es/ecma262/#array-index

Razican · 2022-06-20T09:37:01Z

Test262 conformance changes

VM implementation

Test result	main count	PR count	difference
Total	90,619	90,619	0
Passed	56,859	56,861	+2
Ignored	13,788	13,788	0
Failed	19,972	19,970	-2
Panics	0	0	0
Conformance	62.75%	62.75%	+0.00%

Fixed tests (2):

test/built-ins/Reflect/ownKeys/return-on-corresponding-order-large-index.js [strict mode] (previously Failed)
test/built-ins/Reflect/ownKeys/return-on-corresponding-order-large-index.js (previously Failed)

Razican

Looks very good! Thanks for the fix. I added some comments regarding some potential enhancements. I would like to also get inputs from the rest of the team, since some of the options might just be too much, or might not make sense.

Oh, and tests seem to be failing in MacOS and Windows, so these need to be fixed.

Razican · 2022-06-20T09:38:30Z

boa_engine/src/nonmaxu32.rs

+// Safety: `NonMaxU32` does not contain any objects which needs to be traced,
+// so this is safe.
+unsafe impl Trace for NonMaxU32 {
+    unsafe_empty_trace!();
+}


I would suggest to just not implement this, and use #[unsafe_ignore_trace] in the enum (if that's allowed). It should be more performant.

Razican · 2022-06-20T09:41:48Z

boa_engine/src/nonmaxu32.rs

+/// ```
+#[derive(Copy, Clone, Eq, Finalize, PartialEq, Ord, PartialOrd, Hash)]
+#[repr(transparent)]
+pub struct NonMaxU32(u32);


I would suggest the internal type to be not an enum type and be a structure type:

pub struct NonMaxU32 { inner: u32, }

This would disallow direct editing of the internal structure (having to use getters and setters), and changing the internal type would be API compatible.

Talking about changing the internal type, it might be useful to use a NonZeroU32, which is better optimized for space. On the other hand, this would require using a +1 when storing, and a -1 when retrieving, which could cause a small performance penalty. We should test both options.

Sorry for late reply 🙏 Thank you for your suggestion! I will try to work on a fix.

On the other hand, this would require using a +1 when storing, and a -1 when retrieving, which could cause a small performance penalty.

This sounds a good idea to me. I experimented a bit using XOR to save with NonZeroU32 like the nonmax crate does last week but I realized we would need to rev() the iterator because the xor'ed value would be in reversed order. So, I wonder just +1 and -1 would be easier.

I will try it more and share more information here or on Discord 🙏 Thank you!

I think I'm ok with either approach (-1/+1 or XOR).

Razican · 2022-06-20T09:42:46Z

boa_engine/src/nonmaxu32.rs

+    pub const fn get(self) -> u32 {
+        self.0
+    }


Should we also include setters? And operations such as add, sub, mul and so on?

I'm good with adding these methods though, I wonder it also depends on how we want to use it. Do we want to always do calculation with NonMaxU32 directly?

On the other hand, I wonder it might be good to at least add methods like checked_add or saturating_add on NonZeroU32.

WDYT?

Razican · 2022-06-20T09:44:56Z

boa_engine/src/nonmaxu32.rs

+impl fmt::Debug for NonMaxU32 {
+    #[inline]
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        self.get().fmt(f)
+    }
+}


This might be a good implementation for Display, but I would say the Debug implementation shouldn't hide the internal structure. I'm open to suggestions, though.

I tried to follow NonZeroU32 debug though, it sounds fine to me to format it like NonMaxU32 { inner: 42 }.

Agree with Razican

peddermaster2 · 2022-07-22T06:15:03Z

boa_engine/src/nonmaxu32.rs

+impl BitOr for NonMaxU32 {
+    type Output = Self;
+    #[inline]
+    fn bitor(self, rhs: Self) -> Self::Output {
+        // Safety: since `self` and `rhs` are both nonzero, the
+        // result of the bitwise-or will be nonzero.
+        unsafe { NonMaxU32::new_unchecked(self.get() | rhs.get()) }
+    }
+}
+
+impl BitOr<u32> for NonMaxU32 {
+    type Output = Self;
+
+    #[inline]
+    fn bitor(self, rhs: u32) -> Self::Output {
+        // Safety: since `self` is nonzero, the result of the
+        // bitwise-or will be nonzero regardless of the value of
+        // `rhs`.
+        unsafe { NonMaxU32::new_unchecked(self.get() | rhs) }
+    }
+}
+
+impl BitOr<NonMaxU32> for u32 {
+    type Output = NonMaxU32;
+
+    #[inline]
+    fn bitor(self, rhs: NonMaxU32) -> Self::Output {
+        // Safety: since `rhs` is nonzero, the result of the
+        // bitwise-or will be nonzero regardless of the value of
+        // `self`.
+        unsafe { NonMaxU32::new_unchecked(self | rhs.get()) }
+    }
+}


The "Safety" argument and code seems to be copied from the nonzero implementaion and is not correct. (At least the comment is wrong)

RageKnify

Need to introduce +1/-1 or XOR on accesses. fix the safety comments and correct the safety comments.

jasonwilliams · 2022-11-07T11:44:59Z

Hey @CYBAI how is this going do you need any help?

CYBAI · 2022-12-27T05:17:17Z

Hey @CYBAI how is this going do you need any help?

Sorry for very late reply 🙏 I'll try to find some time to address the comments and update the PR after mid-Jan 🙇

raskad · 2023-09-30T18:54:56Z

Added in #3321

CYBAI force-pushed the nonmaxu32-array-index branch from da9d764 to 6559c96 Compare June 20, 2022 07:26

CYBAI added 2 commits June 20, 2022 16:26

Introduce NonMaxU32 type

28957fa

CYBAI force-pushed the nonmaxu32-array-index branch from 6559c96 to f18bf35 Compare June 20, 2022 07:26

Razican requested review from Razican, raskad, HalidOdat, jasonwilliams, jedel1043 and RageKnify June 20, 2022 09:35

Razican added this to the v0.16.0 milestone Jun 20, 2022

Razican added enhancement New feature or request Internal Category for changelog labels Jun 20, 2022

Razican reviewed Jun 20, 2022

View reviewed changes

peddermaster2 reviewed Jul 22, 2022

View reviewed changes

Razican modified the milestones: v0.16.0, v0.17.0 Sep 19, 2022

RageKnify suggested changes Oct 15, 2022

View reviewed changes

Razican modified the milestones: v0.17.0, v0.18.0 Jul 8, 2023

raskad mentioned this pull request Sep 29, 2023

Add NonMaxU32 as integer variant for PropertyKey #3321

Merged

raskad closed this Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce NonMaxU32 for array index #2131

Introduce NonMaxU32 for array index #2131

CYBAI commented Jun 20, 2022

codecov bot commented Jun 20, 2022 •

edited

Loading

Razican commented Jun 20, 2022

Razican left a comment •

edited

Loading

Razican Jun 20, 2022

Razican Jun 20, 2022

CYBAI Jun 26, 2022

RageKnify Oct 15, 2022

Razican Jun 20, 2022

CYBAI Jun 26, 2022 •

edited

Loading

Razican Jun 20, 2022

CYBAI Jun 26, 2022

RageKnify Oct 15, 2022

peddermaster2 Jul 22, 2022

RageKnify left a comment

jasonwilliams commented Nov 7, 2022

CYBAI commented Dec 27, 2022

raskad commented Sep 30, 2023

Introduce NonMaxU32 for array index #2131

Introduce NonMaxU32 for array index #2131

Conversation

CYBAI commented Jun 20, 2022

codecov bot commented Jun 20, 2022 • edited Loading

Codecov Report

Razican commented Jun 20, 2022

Test262 conformance changes

VM implementation

Razican left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CYBAI Jun 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RageKnify left a comment

Choose a reason for hiding this comment

jasonwilliams commented Nov 7, 2022

CYBAI commented Dec 27, 2022

raskad commented Sep 30, 2023

codecov bot commented Jun 20, 2022 •

edited

Loading

Razican left a comment •

edited

Loading

CYBAI Jun 26, 2022 •

edited

Loading