Implement `regress` fixes #478 #686

neeldug · 2020-09-06T23:19:37Z

This Pull Request fixes/closes #478.

It changes the following:

refactors methods using regex's find to use regress's find and matching API
refactors capture methods using regress's capture group system <- TODO

- Initial switch to `regress` crate, refactors all find methods - TODO: implement captures using `regress` API - 5 current errors for captures and boa/src/builtins/regexp/mod.rs:297:33

- TODO: Fix methods using capture groups - Hopefully everything still works with the refactor other than capture groups

neeldug · 2020-09-08T13:11:34Z

@Razican was wondering whether you had any idea on how to refactor the capture groups that are failing here. Specifically in boa/src/builtins/regexp/mod.rs

RageKnify · 2020-09-08T15:21:24Z

I've managed to make it compile, but the tests are failing, I think the creation of the Regex struct needs to change.
I'll get back to this in a few hours.

My work is here: https://github.com/RageKnify/boa/tree/regress

Razican · 2020-09-08T17:15:03Z

@Razican was wondering whether you had any idea on how to refactor the capture groups that are failing here. Specifically in boa/src/builtins/regexp/mod.rs

I would need to give it a look, but I don't think I will have the time this week :/

Test: Improve string::tests::replace_with_function and fix minor typo Can be improved once ridiculousfish/regress#7 is closed

RageKnify · 2020-09-09T03:00:41Z

@neeldug I believe my branch will finish the PR, it passes cargo test.
Would you like it if I opened a PR to your branch or can you rebase it yourself?

neeldug · 2020-09-09T08:14:53Z

I don’t mind @RageKnify, I’ll rebase it myself since that should be easier.

codecov · 2020-09-09T19:56:28Z

Codecov Report

Merging #686 into master will decrease coverage by 1.48%.
The diff coverage is 72.00%.

@@            Coverage Diff             @@
##           master     #686      +/-   ##
==========================================
- Coverage   73.17%   71.69%   -1.49%     
==========================================
  Files         194      199       +5     
  Lines       14029    13859     -170     
==========================================
- Hits        10266     9936     -330     
- Misses       3763     3923     +160

Impacted Files	Coverage Δ
boa/src/builtins/regexp/mod.rs	`73.15% <69.44%> (-0.10%)`	⬇️
boa/src/builtins/string/mod.rs	`50.47% <75.00%> (-0.48%)`	⬇️
boa/src/builtins/string/tests.rs	`100.00% <100.00%> (ø)`
boa/src/builtins/date/tests.rs	`99.18% <0.00%> (-0.12%)`	⬇️
boa/src/class.rs	`0.00% <0.00%> (ø)`
boa/src/realm.rs	`92.85% <0.00%> (ø)`
boa_wasm/src/lib.rs	`0.00% <0.00%> (ø)`
boa/src/exec/tests.rs	`100.00% <0.00%> (ø)`
boa/examples/classes.rs	`0.00% <0.00%> (ø)`
... and 94 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 76aba2a...aa5c73e. Read the comment docs.

Match::groups simplifies some code.

RageKnify · 2020-09-11T01:14:18Z

Sorry to bother @neeldug, but I improved regress's API and simplified boa's code a bit: 812fcee, when you can rebase it please.
I'm not sure how to improve the coverage, RegExp is decently covered.

- Initial switch to `regress` crate, refactors all find methods - TODO: implement captures using `regress` API - 5 current errors for captures and boa/src/builtins/regexp/mod.rs:297:33

- TODO: Fix methods using capture groups - Hopefully everything still works with the refactor other than capture groups

Test: Improve string::tests::replace_with_function and fix minor typo Can be improved once ridiculousfish/regress#7 is closed

Match::groups simplifies some code.

neeldug · 2020-09-23T15:26:24Z

Ran a rebase for benchmarking to run crit-cmp to see if regress's performance is similar to regex.

HalidOdat · 2020-09-24T19:00:56Z

┌─────────┬──────────────────────────────────────────────┬──────────────────────┬──────────────────────┬────────────┐
│ (index) │                     name                     │   changesDuration    │    masterDuration    │ difference │
├─────────┼──────────────────────────────────────────────┼──────────────────────┼──────────────────────┼────────────┤
│    0    │     'Arithmetic operations (Execution)'      │   '379.9±17.17ns'    │   '381.6±22.38ns'    │   '0.0'    │
│    1    │        'Arithmetic operations (Full)'        │ '**261.7±14.85µs**'  │   '284.1±23.77µs'    │   '-8.3'   │
│    2    │          'Array access (Execution)'          │     '8.9±0.49µs'     │   '**8.8±0.66µs**'   │   '+1.0'   │
│    3    │            'Array access (Full)'             │ '**292.2±12.89µs**'  │   '309.6±24.35µs'    │   '-5.7'   │
│    4    │         'Array creation (Execution)'         │     '3.3±0.14ms'     │   '**3.2±0.10ms**'   │   '+3.0'   │
│    5    │           'Array creation (Full)'            │   '**3.5±0.11ms**'   │     '3.6±0.17ms'     │   '-2.9'   │
│    6    │           'Array pop (Execution)'            │  '1184.7±115.85µs'   │ '**1131.9±60.09µs**' │   '+5.0'   │
│    7    │              'Array pop (Full)'              │ '**1585.7±47.42µs**' │  '1680.9±111.45µs'   │   '-5.7'   │
│    8    │     'Boolean Object Access (Execution)'      │   '**4.9±0.24µs**'   │     '5.2±0.28µs'     │   '-3.8'   │
│    9    │        'Boolean Object Access (Full)'        │ '**275.6±10.44µs**'  │   '287.3±15.17µs'    │   '-3.8'   │
│   10    │            'Clean js (Execution)'            │ '**762.0±36.29µs**'  │   '864.6±171.29µs'   │   '-12'    │
│   11    │              'Clean js (Full)'               │ '**1075.3±98.32µs**' │   '1087.7±57.34µs'   │  '-0.99'   │
│   12    │             'Clean js (Parser)'              │  '**39.0±1.75µs**'   │    '39.3±2.58µs'     │  '-0.99'   │
│   13    │                'Create Realm'                │ '**481.9±20.59ns**'  │   '485.7±21.76ns'    │  '-0.99'   │
│   14    │ 'Dynamic Object Property Access (Execution)' │     '5.9±0.34µs'     │   '**5.6±0.30µs**'   │   '+5.0'   │
│   15    │   'Dynamic Object Property Access (Full)'    │ '**282.3±12.93µs**'  │   '291.0±10.76µs'    │   '-2.9'   │
│   16    │            'Expression (Parser)'             │     '7.1±0.62µs'     │   '**6.9±0.28µs**'   │   '+4.0'   │
│   17    │           'Fibonacci (Execution)'            │   '930.1±44.44µs'    │ '**903.8±34.10µs**'  │   '+3.0'   │
│   18    │              'Fibonacci (Full)'              │ '**1211.8±78.61µs**' │   '1246.9±93.71µs'   │   '-2.9'   │
│   19    │            'For loop (Execution)'            │    '24.3±1.62µs'     │  '**23.8±1.54µs**'   │   '+2.0'   │
│   20    │              'For loop (Full)'               │ '**306.2±14.02µs**'  │   '318.1±22.39µs'    │   '-3.8'   │
│   21    │             'For loop (Parser)'              │    '19.1±0.82µs'     │  '**19.0±1.69µs**'   │   '+1.0'   │
│   22    │           'Goal Symbols (Parser)'            │  '**13.1±2.02µs**'   │    '13.3±1.98µs'     │   '-2.0'   │
│   23    │            'Hello World (Parser)'            │   '**3.2±0.17µs**'   │     '3.3±0.28µs'     │   '-2.0'   │
│   24    │             'Long file (Parser)'             │ '**815.8±35.56ns**'  │   '823.7±53.28ns'    │  '-0.99'   │
│   25    │            'Mini js (Execution)'             │ '**675.9±36.13µs**'  │   '735.7±94.76µs'    │   '-8.3'   │
│   26    │               'Mini js (Full)'               │ '**967.5±31.01µs**'  │   '1008.6±41.26µs'   │   '-3.8'   │
│   27    │              'Mini js (Parser)'              │    '36.2±4.54µs'     │  '**34.0±1.40µs**'   │   '+7.0'   │
│   28    │      'Number Object Access (Execution)'      │   '**4.0±0.19µs**'   │     '4.0±0.18µs'     │   '-2.0'   │
│   29    │        'Number Object Access (Full)'         │ '**275.2±31.94µs**'  │   '289.0±12.53µs'    │   '-4.8'   │
│   30    │        'Object Creation (Execution)'         │     '5.1±0.46µs'     │   '**5.0±0.35µs**'   │   '+2.0'   │
│   31    │           'Object Creation (Full)'           │ '**278.7±18.04µs**'  │   '295.4±20.03µs'    │   '-5.7'   │
│   32    │             'RegExp (Execution)'             │   '**9.9±0.39µs**'   │    '72.0±6.78µs'     │   '-86'    │
│   33    │               'RegExp (Full)'                │ '**289.1±15.70µs**'  │   '377.4±20.02µs'    │   '-24'    │
│   34    │         'RegExp Literal (Execution)'         │  '**11.2±0.58µs**'   │    '80.4±7.74µs'     │   '-86'    │
│   35    │           'RegExp Literal (Full)'            │  '**287.6±8.34µs**'  │   '382.9±22.88µs'    │   '-25'    │
│   36    │    'RegExp Literal Creation (Execution)'     │   '**9.7±0.43µs**'   │    '80.6±15.17µs'    │   '-88'    │
│   37    │       'RegExp Literal Creation (Full)'       │ '**284.7±14.13µs**'  │   '368.7±13.98µs'    │   '-22'    │
│   38    │ 'Static Object Property Access (Execution)'  │     '5.4±0.33µs'     │   '**5.2±0.22µs**'   │   '+4.0'   │
│   39    │    'Static Object Property Access (Full)'    │ '**281.9±19.55µs**'  │   '291.0±14.14µs'    │   '-2.9'   │
│   40    │      'String Object Access (Execution)'      │   '**7.4±0.35µs**'   │     '7.4±0.56µs'     │  '-0.99'   │
│   41    │        'String Object Access (Full)'         │ '**281.6±10.04µs**'  │   '292.8±14.04µs'    │   '-3.8'   │
│   42    │       'String comparison (Execution)'        │   '**6.5±0.20µs**'   │     '6.9±0.39µs'     │   '-6.5'   │
│   43    │          'String comparison (Full)'          │  '**277.7±7.29µs**'  │   '293.4±12.99µs'    │   '-5.7'   │
│   44    │      'String concatenation (Execution)'      │   '**5.4±0.67µs**'   │     '5.5±0.36µs'     │   '-2.0'   │
│   45    │        'String concatenation (Full)'         │  '**273.1±6.76µs**'  │   '292.2±24.97µs'    │   '-6.5'   │
│   46    │          'String copy (Execution)'           │     '4.2±0.16µs'     │     '4.2±0.41µs'     │   '0.0'    │
│   47    │             'String copy (Full)'             │ '**269.0±12.84µs**'  │   '285.7±11.76µs'    │   '-5.7'   │
│   48    │            'Symbols (Execution)'             │     '3.5±0.22µs'     │   '**3.4±0.29µs**'   │   '+2.0'   │
│   49    │               'Symbols (Full)'               │ '**250.1±10.33µs**'  │   '276.1±27.77µs'    │   '-9.1'   │
│   50    │                      ''                      │      undefined       │      undefined       │   '+NaN'   │
└─────────┴──────────────────────────────────────────────┴──────────────────────┴──────────────────────┴────────────┘

Here are the benchmarks you can access this by looking at the benchmark CI log, There seems to be a really big boost in RegExp performance!

│   32    │             'RegExp (Execution)'             │   '**9.9±0.39µs**'   │    '72.0±6.78µs'     │   '-86'    │
│   33    │               'RegExp (Full)'                │ '**289.1±15.70µs**'  │   '377.4±20.02µs'    │   '-24'    │
│   34    │         'RegExp Literal (Execution)'         │  '**11.2±0.58µs**'   │    '80.4±7.74µs'     │   '-86'    │
│   35    │           'RegExp Literal (Full)'            │  '**287.6±8.34µs**'  │   '382.9±22.88µs'    │   '-25'    │
│   36    │    'RegExp Literal Creation (Execution)'     │   '**9.7±0.43µs**'   │    '80.6±15.17µs'    │   '-88'    │
│   37    │       'RegExp Literal Creation (Full)'       │ '**284.7±14.13µs**'  │   '368.7±13.98µs'    │   '-22'    |

HalidOdat · 2020-09-25T13:40:41Z

Why are there old commit changes showing?

neeldug · 2020-09-25T14:21:58Z

I ran a rebase since the criterion versions were mismatched causing benchmarking to fail

HalidOdat · 2020-09-25T14:27:01Z

I ran a rebase since the criterion versions were mismatched causing benchmarking to fail

strange , I think changing the merge base to a different one and back to master may fix this

RageKnify · 2020-09-25T17:04:45Z

I think creating a new branch from master, cherry-picking the original 5 commits onto it. And opening a new PR might me an easier solution, there's not much discussion to be lost anyways and the new PR can mention this one. What do you think @HalidOdat ?

neeldug · 2020-09-25T23:39:08Z

@RageKnify it seems to be sorted now though?

RageKnify · 2020-09-26T10:06:06Z

GitHub seems to think we're merging 11 commits into master and that's not the case at all. There are only 5 new commits and a 6th one for merging (which could not exist if a new branch was made). I think in blame it will all point to this PR, but I still don't like it.

HalidOdat · 2020-09-26T12:40:30Z

GitHub seems to think we're merging 11 commits into master and that's not the case at all. There are only 5 new commits and a 6th one for merging (which could not exist if a new branch was made). I think in blame it will all point to this PR, but I still don't like it.

This shouldn't be a problem since we always squash and merge the PRs into master so, on the master branch all the changes will be represented by one commit (Implement regress fixes #478 (#686) the PR title)

RageKnify · 2020-10-02T23:02:21Z

@neeldug resolve the conflicts when you can please

neeldug · 2020-10-03T00:13:15Z

@RageKnify accidentally deleted fork while this PR was active, have created a new PR with all of the previous commits at #774

neeldug added 2 commits September 7, 2020 00:13

Implement regress fixes #478

659359c

- Initial switch to `regress` crate, refactors all find methods - TODO: implement captures using `regress` API - 5 current errors for captures and boa/src/builtins/regexp/mod.rs:297:33

Fixes a few errors

df40b21

- TODO: Fix methods using capture groups - Hopefully everything still works with the refactor other than capture groups

Fix: Compile with regress #686, failing tests

ec81216

RageKnify mentioned this pull request Sep 9, 2020

Feature: Iterate over the capture groups of a match ridiculousfish/regress#7

Closed

Fix: Cleanup #686, finish transition to regress

532ba6c

Test: Improve string::tests::replace_with_function and fix minor typo Can be improved once ridiculousfish/regress#7 is closed

neeldug marked this pull request as ready for review September 9, 2020 19:48

Refactor: Bump regress from 0.1.3 to 0.1.4

812fcee

Match::groups simplifies some code.

neeldug and others added 6 commits September 23, 2020 16:22

Implement regress fixes #478

92af93b

- Initial switch to `regress` crate, refactors all find methods - TODO: implement captures using `regress` API - 5 current errors for captures and boa/src/builtins/regexp/mod.rs:297:33

Fixes a few errors

329e2b3

- TODO: Fix methods using capture groups - Hopefully everything still works with the refactor other than capture groups

Fix: Compile with regress #686, failing tests

8d6492b

Fix: Cleanup #686, finish transition to regress

dc814c1

Test: Improve string::tests::replace_with_function and fix minor typo Can be improved once ridiculousfish/regress#7 is closed

Refactor: Bump regress from 0.1.3 to 0.1.4

1d9d48d

Match::groups simplifies some code.

Merge branch 'regress' of https://github.com/neeldug/boa into regress

aa5c73e

HalidOdat added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request execution Issues or PRs related to code execution performance Performance related changes and issues labels Sep 24, 2020

HalidOdat requested review from HalidOdat, Razican, Lan2u and jasonwilliams September 24, 2020 19:01

HalidOdat added this to the v0.10.0 milestone Sep 24, 2020

neeldug changed the base branch from master to feature/548 September 25, 2020 14:49

neeldug changed the base branch from feature/548 to master September 25, 2020 14:49

jasonwilliams modified the milestones: v0.10.0, v0.11.0 Sep 29, 2020

HalidOdat approved these changes Oct 2, 2020

View reviewed changes

neeldug closed this Oct 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `regress` fixes #478 #686

Implement `regress` fixes #478 #686

neeldug commented Sep 6, 2020

neeldug commented Sep 8, 2020 •

edited

Loading

RageKnify commented Sep 8, 2020

Razican commented Sep 8, 2020

RageKnify commented Sep 9, 2020

neeldug commented Sep 9, 2020

codecov bot commented Sep 9, 2020 •

edited

Loading

RageKnify commented Sep 11, 2020 •

edited

Loading

neeldug commented Sep 23, 2020

HalidOdat commented Sep 24, 2020

HalidOdat commented Sep 25, 2020

neeldug commented Sep 25, 2020

HalidOdat commented Sep 25, 2020

RageKnify commented Sep 25, 2020

neeldug commented Sep 25, 2020

RageKnify commented Sep 26, 2020

HalidOdat commented Sep 26, 2020

RageKnify commented Oct 2, 2020

neeldug commented Oct 3, 2020

Implement regress fixes #478 #686

Implement regress fixes #478 #686

Conversation

neeldug commented Sep 6, 2020

neeldug commented Sep 8, 2020 • edited Loading

RageKnify commented Sep 8, 2020

Razican commented Sep 8, 2020

RageKnify commented Sep 9, 2020

neeldug commented Sep 9, 2020

codecov bot commented Sep 9, 2020 • edited Loading

Codecov Report

RageKnify commented Sep 11, 2020 • edited Loading

neeldug commented Sep 23, 2020

HalidOdat commented Sep 24, 2020

HalidOdat commented Sep 25, 2020

neeldug commented Sep 25, 2020

HalidOdat commented Sep 25, 2020

RageKnify commented Sep 25, 2020

neeldug commented Sep 25, 2020

RageKnify commented Sep 26, 2020

HalidOdat commented Sep 26, 2020

RageKnify commented Oct 2, 2020

neeldug commented Oct 3, 2020

Implement `regress` fixes #478 #686

Implement `regress` fixes #478 #686

neeldug commented Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 9, 2020 •

edited

Loading

RageKnify commented Sep 11, 2020 •

edited

Loading