Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey wanted-standalone-js)

Attachments

(9 attachments, 8 obsolete attachments)

1.20 MB, patch
cdleary
: review+
Details | Diff | Splinter Review
13.92 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
4.73 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
4.46 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
5.88 KB, patch
dvander
: review+
Details | Diff | Splinter Review
284.81 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
135.08 KB, patch
cdleary
: review+
Details | Diff | Splinter Review
1.51 MB, patch
Details | Diff | Splinter Review
231.68 KB, application/x-zip-compressed
Details
(Assignee)

Description

7 years ago
I don't think this should block for now; we should only block on things that are proven to affect the web. But it's fairly important and we should do as much of this as time permits before Fx4. We should probably start by surveying all the changes that landed since Chris's last import, seeing if they fix things that look important and if they caused regressions, and go from there.
No longer blocks: 625502
Blocks: 625502
No longer blocks: 625502
(Assignee)

Comment 1

7 years ago
Preliminary report:

Currently, our import of yarr is:

       rev 68100
       disable the yarr interpreter
       plus cherry-picked rev 72781

The revs that change yarr that we have not taken are:

* Fixes

72813 + 73594 - don't match bogus char class + back off a bit for web compat
73617 - something with backtracking
74309 - crash on Yelp
75408 - make syntax errors early errors
75796 - backtracking for nested alternatives
75991 - fuzzer crash
76275 - crash
76407 - hang

* Regression Fixes

73640
73866
73962
76076 + 76133 (followup for regression introduced by 76076)

* Refactorings

72999 - typos
73124
74412
74600
75421
[[75595 + 75597]] (backed-out patch)
75602

* Optimizations

68207 - improve v8-regexp by 2.7%
72180
73307
73903
74441 - reduce cache mem use
76502

* Interpreter

68127
68771
69781
69842 + 69847 (build fix for 69842)
70764
72140
72186
[[72197 + 72781]] (backed-out patch)
(Assignee)

Comment 2

7 years ago
I think the next step is to go through the fixes and regression fixes and see which we are currently affected by. Then we can try to import the patches that fix those. But we need to check for regressions from them, and see which additional patches we would need to import in order to import those patches.
(Assignee)

Comment 3

7 years ago
For all the fixes (not including regression fixes) that have obvious test cases, we currently pass those tests. So I think we are pretty good here and don't need to do anything else before Fx4.
(Assignee)

Updated

7 years ago
Blocks: 638208
(Assignee)

Updated

7 years ago
Assignee: general → dmandelin
(Assignee)

Comment 4

7 years ago
I'm working on this now. It's probably going to take a week or two, partly because it's been a few months since we've updated so there's a lot to go through, and partly because I'll be gone for a day or two for a conference next week.
(Assignee)

Updated

7 years ago
Blocks: 644143
(Assignee)

Updated

7 years ago
Blocks: 633677
(Assignee)

Updated

6 years ago
Blocks: 652909
(Assignee)

Comment 5

6 years ago
Starting this up again. In order to make future merging easier and upstreaming possible, I want to make minimal changes to the WebKit code, and instead add a bridge that does things like implement the WebKit Vector interface on top of our implementation. Most of this should not be too hard, but there is a sticking point:

The JSC/Yarr code uses refcounting smart pointers quite a bit: a RefCounted template, RefPtr and PassRefPtr pointers, etc. Ideally, we would just provide our own implementation of enough of this stuff to make it work.

The problem is that the refcounting template code in WebKit is LGPL 2+, which, IIUC, is not compatible with the tri-license and cannot be imported into our tree. What can I do?

Can I reimplement that functionality myself? I have looked at those files a little bit, e.g., to understand what they are for and to check on their license.
I can confirm that LGPL2+ is not permitted by Mozilla licensing policy (and won't be even when/if we move to MPL 2).

Gerv
(Assignee)

Updated

6 years ago
Blocks: 654693
(Assignee)

Updated

6 years ago
Blocks: 653672
(Assignee)

Updated

6 years ago
Depends on: 656509
(Assignee)

Comment 7

6 years ago
Created attachment 532027 [details] [diff] [review]
WIP

Builds on Windows but asserts if you try to run a regexp--lots of fixmes left. The good news is that I was able to get it to build making relatively few changes to the Yarr code. I think I will make Yarr use our hash tables, though--they only use a hash table in one place, and the API impedance mismatch is pretty bad.
(Assignee)

Comment 8

6 years ago
Created attachment 532278 [details] [diff] [review]
WIP 2

This works for the most part on Windows. There are some jit-test failures. One is bug 576837, which I had thought was fixed upstream, but apparently not. I can just take our fix and hopefully upstream that as well. There are some other failures that seem to work correctly in the latest WebKit, so I'll have to update to that. There are also about 20 other issues to check on, such as Mac/Linux testing and header file cleanups.
Attachment #532027 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 532352 [details] [diff] [review]
WIP 3

Passes jit-tests on Windows. Now I just need to crank through 20 or so engineering issues. The only hard one is memory management--it probably leaks right now and needs some replacement for the refcounting smart pointers.
Attachment #532278 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 532379 [details] [diff] [review]
WIP 4

What's left is to be fixed up for Mac and Linux, update license files, and fix memory managements.
Attachment #532352 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Created attachment 532764 [details] [diff] [review]
WIP 5

Almost done. I just need to check on Linux compat.
Attachment #532379 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
Created attachment 532816 [details] [diff] [review]
WIP 6

I found a few bugs with jstests, so I had to refresh Yarr, readapt that (which turned out to be very easy with my approach of keeping a separate "adapt Yarr to Moz" patch), and fix another bug that cdleary might have fixed earlier, or else WebKit decided to unfix.

This still needs Linux/Mac testing, but hopefully is really really close now.
Attachment #532764 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 532830 [details] [diff] [review]
Part 1: Remove old yarr files

I'm going to put this up as a series of patches, so that each part of the adaptation is documented. This should also make it easier to review.

Question about landing, though: Do you think I should land it in parts, so that it is permanently documented in the tree, or as one patch, so that it's easier to back out later if that's required?
Attachment #532816 - Attachment is obsolete: true
Attachment #532830 - Flags: review?(cdleary)
(Assignee)

Comment 14

6 years ago
Created attachment 532831 [details]
Part 2: Import Yarr rev 86639 unmodified
Attachment #532831 - Flags: review?(cdleary)
(Assignee)

Comment 15

6 years ago
Created attachment 532832 [details]
Part 3: Adapt Yarr rev 86639 to build in Spidermonkey
Attachment #532832 - Flags: review?(cdleary)
(Assignee)

Comment 16

6 years ago
Created attachment 532833 [details] [diff] [review]
Part 4: Fix Yarr memory management to work with SpiderMonkey

I kept this separate because it's more likely to change than the rest.
Attachment #532833 - Flags: review?(cdleary)
(Assignee)

Comment 17

6 years ago
Created attachment 532834 [details] [diff] [review]
Part 5: Fix bug 230216

This bug was latent in Yarr rev 86639. It is not present in our initial import; I don't know if cdleary fixed it or if Yarr had it right at that point.
Attachment #532834 - Flags: review?(cdleary)
(Assignee)

Comment 18

6 years ago
Created attachment 532835 [details] [diff] [review]
Part 6: Fix bug 576837

cdleary fixed this in the original import; this is just an update of his fix. Yarr previously fixed this bug, but they fixed it too hard (too restrictive), so they undid the fix.
Attachment #532835 - Flags: review?(cdleary)
(Assignee)

Comment 19

6 years ago
Created attachment 532837 [details] [diff] [review]
Part 7: Optional API rename to match JSC

This is optional; it's not needed to fix this bug correctly. We had previously added 'isValid' methods to some Nitro asm jump holder classes. Since then, they have added 'isSet'. This just renames one of our 'isValid' to match their 'isSet', in a class that would otherwise need both APIs.
Attachment #532837 - Flags: review?(dvander)
(Assignee)

Comment 20

6 years ago
Created attachment 532838 [details] [diff] [review]
Part 2 (fixed): Import Yarr rev 86639 unmodified
Attachment #532838 - Flags: review?(cdleary)
(Assignee)

Updated

6 years ago
Attachment #532831 - Attachment is obsolete: true
Attachment #532831 - Flags: review?(cdleary)
(Assignee)

Comment 21

6 years ago
Created attachment 532841 [details] [diff] [review]
Part 3 (fixed): Adapt Yarr rev 86639 to build in Spidermonkey
Attachment #532832 - Attachment is obsolete: true
Attachment #532841 - Flags: review?(cdleary)
Attachment #532832 - Flags: review?(cdleary)
Blocks: 657804
(Assignee)

Updated

6 years ago
Blocks: 605998
Attachment #532837 - Flags: review?(dvander) → review+
Comment on attachment 532830 [details] [diff] [review]
Part 1: Remove old yarr files

A few comments:

- I'm guessing that all of the FIXMEs are imported and left in for ease of merging from upstream.
- I would make sure this all builds with --disable-methodjit (I see you added the include of "methodjit/MethodJIT.h" in the inlines file).
- I'd prefer it if we could keep the android bug-reference comment -- ideally that EnableYarrJIT wouldn't be there at all.
- executeInternal has an awkwardly expressed JS_ASSERT(result >= 0) with JS_ASSERT(0)s in it.
- "const size_t notFound = static_cast<size_t>(-1);" Does the static cast buy you anything here? I think signed -1 should just signex to fill the size_t.
- In compileHelper you have to JS_ASSERT(0) if !ENABLE_YARR_JIT. Maybe this will be changed in the rest of the patches?

I'm not sure this is something we can commit piecemeal, since all the prior YARR files are ripped out. Making changesets which can compile and run the full gamut of tests is the rule of thumb I'm familiar with.
Attachment #532830 - Flags: review?(cdleary) → review+
Comment on attachment 532838 [details] [diff] [review]
Part 2 (fixed): Import Yarr rev 86639 unmodified

Not having reviewed all the constituent changesets on the webkit side, rs=me.
Attachment #532838 - Flags: review?(cdleary) → review+
Comment on attachment 532841 [details] [diff] [review]
Part 3 (fixed): Adapt Yarr rev 86639 to build in Spidermonkey

These char-error interfaces introduce some unfortunate merge-from-upstream issues -- maybe we can get those a little more loosely coupled upstream in the future.

One note: it looks like some of your Makefiles are replacing tabstops with 4 softabstop, in the prior patches as well, which I forgot to mention.
Attachment #532841 - Flags: review?(cdleary) → review+
Comment on attachment 532833 [details] [diff] [review]
Part 4: Fix Yarr memory management to work with SpiderMonkey

There's one "// YYY" comment that may need to be removed. When I first implemented the CharTable ownership there were a few tricky leaks -- this doesn't look different from the way it was, but if it happens to be or you're feeling extra cautious I would give the /regexp/i reftests a run through valgrind.
Attachment #532833 - Flags: review?(cdleary) → review+
Attachment #532834 - Flags: review?(cdleary) → review+
Attachment #532835 - Flags: review?(cdleary) → review+
(Assignee)

Comment 26

6 years ago
Landed with a few nit-fixes added.

http://hg.mozilla.org/tracemonkey/rev/de6dfe16fd91
(Assignee)

Comment 27

6 years ago
Backed out: http://hg.mozilla.org/tracemonkey/rev/6422857800fe

I think I bumped into the bug changing the new ctors. Will push a corrected version soon.
(Assignee)

Comment 28

6 years ago
Relanded:

http://hg.mozilla.org/tracemonkey/rev/195f8cf8758f
(Assignee)

Comment 29

6 years ago
Rebacked out. I already bounced this off TM a few times today, thinking that maybe there was just a small issue or 2. But there is some jsvector re-entrance issue, and some ARM build stuff, that for some reason didn't pop on try. So I'm not sure if this will be able to land before tomorrow. I'm not sure that a 2MB that lands just before the branch would be likely to get through Aurora without getting backed out, anyway. But we'll see.
(Assignee)

Updated

6 years ago
Depends on: 659210
(Assignee)

Comment 30

6 years ago
Created attachment 534835 [details] [diff] [review]
Updated version

Takes care of issues introduced by bug 657384 and works correctly on no-jit builds. There are still some Android build issues to be sorted out.
(Assignee)

Comment 31

6 years ago
Looks like it finally stuck:

http://hg.mozilla.org/tracemonkey/rev/cc36a234d0d6
http://hg.mozilla.org/tracemonkey/rev/c8695a65e1e7
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 32

6 years ago
Created attachment 535489 [details]
Yarr import patch queue

This is a zip of the corrected patch queue I used to do this Yarr import. It has these patches, in this order:

  del-old-yarr.diff
      Deletes the old Yarr files, from the first import, to get a clean tree.

  import-yarr-rev-86639.diff
      Copies in Yarr files from the given WebKit rev, and nothing more.

  adapt-yarr-rev-86639.diff
      Adapts Yarr and SpiderMonkey to each other, except for memory management
      issues, because those are more likely to change.
  fix-mem.diff
      Adapt memory management.

  fix-number.diff
      Fix a Yarr bug with detecting out-of-range numeric inputs.
  fix-cc.diff
      Fix a Yarr bug with being too permissive on character classes

  opt-isset.diff
      Refactor a bit of our imported Yarr assembler to more closely match WebKit.

The idea here is that the next refresh of Yarr rev R can be done like this:

1. Delete the current Yarr files. Name this patch del-yarr-rev-86639.diff.
2. Copy in Yarr files for rev R. Name this patch import-yarr-rev-R.diff.
3. Apply the rest of the patches in order, fixing them as necessary and updating.

I actually had to do this during my work on this bug, because I discovered a bug in Yarr that had been fixed a few days after my import. I found that the patch queue applied pretty cleanly.
(Assignee)

Updated

6 years ago
Depends on: 660371

Updated

6 years ago
Depends on: 660591
Depends on: 660747
(Assignee)

Updated

6 years ago
No longer depends on: 660371
(Assignee)

Updated

6 years ago
Depends on: 661973
Blocks: 661974
No longer blocks: 661974
Blocks: 576828
(Assignee)

Updated

6 years ago
Duplicate of this bug: 657804
(Assignee)

Updated

6 years ago
Duplicate of this bug: 654693
(Assignee)

Updated

6 years ago
Duplicate of this bug: 652909
(Assignee)

Updated

6 years ago
Duplicate of this bug: 644143
(Assignee)

Updated

6 years ago
Duplicate of this bug: 633677
(Assignee)

Updated

6 years ago
Duplicate of this bug: 664527
http://hg.mozilla.org/mozilla-central/rev/cc36a234d0d6
http://hg.mozilla.org/mozilla-central/rev/c8695a65e1e7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 670031

Updated

6 years ago
Depends on: 670003

Comment 41

6 years ago
Where is this fixed? Please set status flags and Target Milestone

Updated

6 years ago
Depends on: 679986
Depends on: 682252
Depends on: 682572

Updated

6 years ago
Depends on: 683838

Updated

6 years ago
Depends on: 679936
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla8
Depends on: 685477
Depends on: 687302

Updated

6 years ago
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey wanted-standalone-js

Updated

6 years ago
Depends on: 691172

Updated

5 years ago
Depends on: 751124

Updated

5 years ago
Depends on: 756838

Updated

4 years ago
Depends on: 885299

Updated

4 years ago
Depends on: 898189

Updated

4 years ago
Depends on: 938020
You need to log in before you can comment on or make changes to this bug.