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

Implement Array.prototype.flat/flatMap #1132

Merged
merged 4 commits into from
May 10, 2021

Conversation

davimiku
Copy link
Contributor

This Pull Request fixes part of #36 .

It changes the following:

  • Implements Array.prototype.flat
  • Implements Array.prototype.flatMap
  • Implements abstract function FlattenIntoArray
  • Adds test for both builtins

A few things I wasn't sure about:

  • I was pretty literal with following the spec, I can tone that back a bit
  • The FlattenIntoArray abstract method is supposed to return a "non-negative integer" (it's called recursively and this return value determines when the recursion ends). I had it return a Value::Integer for convenience but I didn't know if it should maybe return a primitive u32 instead
  • I have a value compare against Number::MAX_SAFE_INTEGER per the spec, but it's a u32 so couldn't be larger than 2^53 anyways. I didn't test any arrays with huge numbers of elements so there's probably missed edge cases
  • I don't think it deals with empty elements in arrays (i.e.[1, , 3]), I wasn't sure if those have a representation in Boa yet

@davimiku davimiku mentioned this pull request Feb 24, 2021
33 tasks
@RageKnify RageKnify self-requested a review March 29, 2021 19:54
Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

Code looks good. I have one correction and one suggestion that might not be possible.

There's also something else:

let a = [0, 1, 2]
delete a[1]
a.flatMap(x => { return [a] })

I think it would return [0, undefined, 2] when it should just be [0, 2]. (edit: Just noticed that you mentioned knowing that, I'd say to leave coment with TODO mentioning that topic so it can be spotted in the future)

As an heads-up you'll need to merge master into your branch to respect the new lints from Rust 1.51.

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
@RageKnify RageKnify added this to the v0.12.0 milestone Mar 30, 2021
@RageKnify RageKnify added the builtins PRs and Issues related to builtins/intrinsics label Mar 30, 2021
- Implements Array.prototype.flat
- Implements Array.prototype.flatMap
- Implements abstract function FlattenIntoArray
- Adds test for both builtins
- Register function with number of required args
- Simplify match statement for depth check
- Add check and test for empty element in array
@davimiku davimiku force-pushed the feat/builtins-array-flat-flatmap branch from 90fab8b to 52b11e2 Compare April 1, 2021 02:45
@davimiku
Copy link
Contributor Author

davimiku commented Apr 1, 2021

I added a check and test for an array with "holes" or empty elements, so that part should be working now.

I rebased this branch onto master which I think should cover the clippy checks -- but I'm not totally sure (new to Rust and still getting used to the ecosystem).

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

Looks good I like it. @davimiku im curious to know what your experience was like implementing this. Was it easy/difficult? Did you look at how other methods worked to gain an understanding? Is there something Boa should be doing better to make it easier?

I was pretty literal with following the spec, I can tone that back a bit

This is a good example of following the spec so we'll keep it around as a use-case. But yeah you don't need to be as literal as long as you can pass test262 there's usually optimizations/shortcuts you can make

I don't think it deals with empty elements in arrays (i.e.[1, , 3]), I wasn't sure if those have a representation in Boa yet

I think you're right to mention this, it looks like as we parse the array literal to generate the AST we count a hole as undefined and replace it with undefined. This happens at parsing time. It looks like smarter engines know the difference between a hole and undefined, this may be a separate issue to investigate and fix.

I have a value compare against Number::MAX_SAFE_INTEGER per the spec, but it's a u32 so couldn't be larger than 2^53 anyways. I didn't test any arrays with huge numbers of elements so there's probably missed edge cases

We can pick this up with further Test 262 in future

@davimiku
Copy link
Contributor Author

davimiku commented Apr 6, 2021

@jasonwilliams Good question - it was straightforward to find where in the codebase to find these functions and the already implemented functions & tests were helpful as a reference. I think the biggest barrier in my case was outside of Boa, it was getting used to how to read the ECMA Spec and learning Rust syntax.

One thing Boa might be able to do for new contributors is describe how it represents Javascript values -- ex. GcObject vs. Object vs. Array (same as Object?), how prototype methods are called, accessing properties, etc. The other thing that was a bit unclear was the boundary between modules, for example I wasn't sure if I should have used the array_iterator module as the iterator for the array builtin functions or just use a 'for' loop (did the latter). I suspect both of those would also clear up more over time.

For the arrays with holes, yeah it sounds like that's probably best as a separate issue. Peeking into Node (V8) internals, there's a different backing store whether the array has holes or not, guessing that there's an iterator used in the array methods that the packed version has an optimization that it doesn't need to check whether the element exists or not.

$ node --allow-natives-syntax
> const arr1 = [1, 2, 3]
> %DebugPrint(arr1)
- elements: <FixedArray[3]> [PACKED_SMI_ELEMENTS (COW)]

> const arr2 = [1, , 3]
> %DebugPrint(arr2)
 - elements: <FixedArray[3]> [HOLEY_SMI_ELEMENTS (COW)]

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This looks very good. I love how you followed the spec perfectly, and how it's very well documented. Some extra stats:

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 25,346 25,381 +35
Ignored 15,587 15,587 0
Failed 37,564 37,529 -35
Panics 16 16 0
Conformance 32.29 32.33 +0.04%

Benchmarks:

┌─────────┬──────────────────────────────────────────────┬──────────────────────┬─────────────────────┬────────────┐
│ (index) │                     name                     │   changesDuration    │   masterDuration    │ difference │
├─────────┼──────────────────────────────────────────────┼──────────────────────┼─────────────────────┼────────────┤
│    0    │     'Arithmetic operations (Execution)'      │ '**284.9±16.50ns**'  │   '306.0±19.14ns'   │   '-6.5'   │
│    1    │        'Arithmetic operations (Full)'        │   '210.9±13.07µs'    │ '**209.7±12.78µs**' │   '+1.0'   │
│    2    │          'Array access (Execution)'          │   '**5.0±0.33µs**'   │    '5.1±0.32µs'     │   '-2.0'   │
│    3    │            'Array access (Full)'             │ '**226.2±11.44µs**'  │   '231.6±13.34µs'   │   '-2.0'   │
│    4    │         'Array creation (Execution)'         │   '**2.5±0.15ms**'   │    '2.8±0.15ms'     │   '-11'    │
│    5    │           'Array creation (Full)'            │   '**2.5±0.11ms**'   │    '2.8±0.15ms'     │   '-9.9'   │
│    6    │           'Array pop (Execution)'            │ '**792.8±52.52µs**'  │   '865.8±47.31µs'   │   '-8.3'   │
│    7    │              'Array pop (Full)'              │ '**1149.4±72.61µs**' │  '1212.8±75.49µs'   │   '-5.7'   │
│    8    │     'Boolean Object Access (Execution)'      │   '**4.3±0.23µs**'   │    '4.7±0.28µs'     │   '-7.4'   │
│    9    │        'Boolean Object Access (Full)'        │   '246.3±14.47µs'    │ '**238.4±13.17µs**' │   '+3.0'   │
│   10    │            'Clean js (Execution)'            │ '**559.8±30.35µs**'  │   '584.7±41.42µs'   │   '-3.8'   │
│   11    │              'Clean js (Full)'               │   '848.2±49.99µs'    │ '**835.5±41.76µs**' │   '+2.0'   │
│   12    │             'Clean js (Parser)'              │    '36.2±2.05µs'     │  '**34.7±1.76µs**'  │   '+5.0'   │
│   13    │                'Create Realm'                │   '387.0±21.72ns'    │ '**380.1±21.35ns**' │   '+2.0'   │
│   14    │ 'Dynamic Object Property Access (Execution)' │   '**4.1±0.26µs**'   │    '4.2±0.26µs'     │   '-2.0'   │
│   15    │   'Dynamic Object Property Access (Full)'    │ '**219.4±10.88µs**'  │   '243.5±14.81µs'   │   '-9.9'   │
│   16    │            'Expression (Parser)'             │     '6.0±0.39µs'     │    '6.0±0.34µs'     │   '0.0'    │
│   17    │           'Fibonacci (Execution)'            │ '**676.5±34.91µs**'  │   '707.8±46.72µs'   │   '-4.8'   │
│   18    │              'Fibonacci (Full)'              │ '**904.0±51.96µs**'  │   '929.1±53.77µs'   │   '-2.9'   │
│   19    │            'For loop (Execution)'            │  '**18.4±1.20µs**'   │    '20.4±0.94µs'    │   '-9.9'   │
│   20    │              'For loop (Full)'               │ '**242.3±15.58µs**'  │   '252.9±15.14µs'   │   '-3.8'   │
│   21    │             'For loop (Parser)'              │    '18.1±1.56µs'     │  '**17.8±0.99µs**'  │   '+2.0'   │
│   22    │           'Goal Symbols (Parser)'            │  '**12.5±0.91µs**'   │    '12.6±0.81µs'    │  '-0.99'   │
│   23    │            'Hello World (Parser)'            │   '**3.2±0.21µs**'   │    '3.3±0.21µs'     │   '-2.9'   │
│   24    │             'Long file (Parser)'             │ '**693.0±41.36ns**'  │   '714.0±35.85ns'   │   '-2.9'   │
│   25    │            'Mini js (Execution)'             │ '**485.0±24.76µs**'  │   '526.0±33.36µs'   │   '-7.4'   │
│   26    │               'Mini js (Full)'               │ '**795.0±42.01µs**'  │   '810.4±50.69µs'   │   '-2.0'   │
│   27    │              'Mini js (Parser)'              │  '**30.7±1.81µs**'   │    '31.4±1.84µs'    │   '-2.0'   │
│   28    │      'Number Object Access (Execution)'      │   '**3.4±0.16µs**'   │    '3.6±0.25µs'     │   '-5.7'   │
│   29    │        'Number Object Access (Full)'         │   '239.8±13.34µs'    │ '**226.4±13.58µs**' │   '+6.0'   │
│   30    │        'Object Creation (Execution)'         │   '**3.6±0.21µs**'   │    '3.6±0.27µs'     │  '-0.99'   │
│   31    │           'Object Creation (Full)'           │   '221.2±16.51µs'    │ '**214.3±13.14µs**' │   '+3.0'   │
│   32    │             'RegExp (Execution)'             │   '**7.9±0.34µs**'   │    '8.5±0.60µs'     │   '-7.4'   │
│   33    │               'RegExp (Full)'                │   '249.2±15.68µs'    │ '**242.8±14.00µs**' │   '+3.0'   │
│   34    │         'RegExp Literal (Execution)'         │     '9.3±0.61µs'     │    '9.3±0.55µs'     │   '0.0'    │
│   35    │           'RegExp Literal (Full)'            │   '250.0±16.28µs'    │ '**230.0±16.05µs**' │   '+9.0'   │
│   36    │    'RegExp Literal Creation (Execution)'     │   '**7.8±0.44µs**'   │    '8.6±0.50µs'     │   '-9.1'   │
│   37    │       'RegExp Literal Creation (Full)'       │ '**225.6±13.51µs**'  │   '235.3±12.65µs'   │   '-3.8'   │
│   38    │ 'Static Object Property Access (Execution)'  │   '**3.8±0.18µs**'   │    '3.9±0.24µs'     │   '-2.9'   │
│   39    │    'Static Object Property Access (Full)'    │  '**219.0±9.76µs**'  │   '225.1±16.01µs'   │   '-2.9'   │
│   40    │      'String Object Access (Execution)'      │   '**5.9±0.34µs**'   │    '6.5±0.36µs'     │   '-8.3'   │
│   41    │        'String Object Access (Full)'         │   '238.9±15.46µs'    │ '**229.1±14.04µs**' │   '+4.0'   │
│   42    │       'String comparison (Execution)'        │   '**5.5±0.29µs**'   │    '6.2±0.34µs'     │   '-9.9'   │
│   43    │          'String comparison (Full)'          │ '**228.6±13.24µs**'  │   '233.7±15.15µs'   │   '-2.0'   │
│   44    │      'String concatenation (Execution)'      │   '**4.4±0.24µs**'   │    '4.7±0.25µs'     │   '-7.4'   │
│   45    │        'String concatenation (Full)'         │   '232.9±14.60µs'    │ '**229.0±14.35µs**' │   '+2.0'   │
│   46    │          'String copy (Execution)'           │   '**3.3±0.21µs**'   │    '3.4±0.21µs'     │   '-2.0'   │
│   47    │             'String copy (Full)'             │ '**210.9±11.55µs**'  │   '216.6±13.72µs'   │   '-2.9'   │
│   48    │            'Symbols (Execution)'             │   '**2.9±0.16µs**'   │    '3.0±0.16µs'     │   '-4.8'   │
│   49    │               'Symbols (Full)'               │ '**205.2±10.83µs**'  │   '220.7±12.14µs'   │   '-7.4'   │
│   50    │                      ''                      │      undefined       │      undefined      │   '+NaN'   │
└─────────┴──────────────────────────────────────────────┴──────────────────────┴─────────────────────┴────────────┘

@Razican Razican merged commit 2f4c47d into boa-dev:master May 10, 2021
Razican pushed a commit that referenced this pull request May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants