740015 - update Yarr import again

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

The newest version of YARR supports some optimizations that are important for benchmarks (see bug 605385, for instance), and it'd be good to try and keep pace with upstream anyway.

This looks like a messy process; it'd be nice to use an update.sh style for this import to cut down on hacks later.  I don't know if there are useful bits in the (well-separated!) patch series from bug 625600 that we could re-use.

Comment 1

[David Mandelin [:dmandelin]]

What's update.sh?

Bug 635600 comment 32 documents my original patch series and the process I had imagined for the next time around, but you may be able to come with something better. Note that a few other Yarr bugfixes have landed since then.

Another idea I had was to start with the rev of import-yarr-rev-86639.diff (call it Y1), apply the Yarr updates to that (call this Y2), and then merge Y2 and tip to form Y3. diff(Y3, Y2) would be the adapt-Yarr-new-to-SpiderMonkey diff. The only downside is that this loses the separation of the adapt-Yarr into memory management, bugfixes, etc. I think you could do a similar thing patch by patch (instead of merging to tip, merge to each applied adapt-Yarr patch) in order to update the individual adaptation patches.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to David Mandelin from comment #1)
> What's update.sh?

http://mxr.mozilla.org/mozilla-central/find?string=/update.sh

A lot of other "import third-party code" directories in the tree have this script, update.sh, that you run like so:

cd /path/to/mozilla/directory && sh update.sh /path/to/directory/to/import

and the appropriate files get copied over, along with applying a patch series not unlike the patch series you posted in bug 625600 comment 32.

> Bug 635600 comment 32 documents my original patch series and the process I
> had imagined for the next time around, but you may be able to come with
> something better. Note that a few other Yarr bugfixes have landed since then.

I don't have any good ideas at the moment, especially as a new version of Yarr probably requires a new version of the assembler, which I assume has had improvements/bugfixes made along the way on our side.  So that's double the work right there.  Whether an assembler import is needed too, it gets ugly, and there are differences that need to be fixed up (mozilla::CheckedInt vs. WTF::Checked APIs, etc.).

I have the rest of this week to work on "performance problems", so I will stare at the code on Thursday and Friday and see if I can't get some semblance of sanity to come out of the merge.

Comment 3

[David Mandelin [:dmandelin]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> I don't have any good ideas at the moment, especially as a new version of
> Yarr probably requires a new version of the assembler, which I assume has
> had improvements/bugfixes made along the way on our side.  So that's double
> the work right there.  Whether an assembler import is needed too, it gets
> ugly, and there are differences that need to be fixed up
> (mozilla::CheckedInt vs. WTF::Checked APIs, etc.).

Last time around, I did spot updates to the assembler. There weren't too many of them, but it still took some time. I think it was mostly the checked stuff.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to David Mandelin from comment #3)
> Last time around, I did spot updates to the assembler. There weren't too
> many of them, but it still took some time. I think it was mostly the checked
> stuff.

Having looked at this today, I can confirm that, yes, updates to the assembler are required.  In addition, it looks like even what we imported and the assembler at the webkit svn revision are somwhat different, which is quite puzzling to me (and also the source of a number of conflicts).

The approach I'm going with for now is simply to import each WebKit revision that touches yarr and the assembler, attempting to keep things compiling along the way.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

CheckedInt has now moved to mfbt/ on inbound.

Note that WebKit is already using CheckedInt and I've pinged a couple people about syncing their copies. (I infer from above discussion that Yarr is something from WebKit but I have no clue).

Comment 6

[Tom S [:evilpie]]

We need somebody to do this. They also landed 8bit chars recently, not sure what role this is going play here.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Dave, is there someone that can be assigned to this? There are way too many non-trivial changes at this point for this to be in my comfort zone to try.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, baseline patch

This patch imports Yarr and some assorted WTF files from WebKit revision 130234, wholesale replacing any files from their prevision revision (86639). It does not contain any Mozilla modifications, so it does not build.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, mozilla adaptations WIP

Most of the cherry-picked changes in the diff of our current tree against the old Yarr revision, were already upstream. So most of the changes were around minor differences:

 (1) Changing CPU()/COMPILER() macros.
 (2) JSLinearString/JSC string differences (like our lack of 8-bit support).
 (3) Things our build system will whine about (extra semicolons, commas).
 (4) new -> js_new
 (5) propagating errors in a few places

This passes jit-tests (didn't try jstests yet). Tomorrow I'll take another pass, clean up some of the #if 0s and unnecessary changes, and do a tbpl run.

A neat feature Yarr has now is that it will can compile two versions of a regex: one for matching only and one for the full output grabbing, which I guess can be used to optimize Regex.test over Regexp.match? I'm not sure yet. I'll file a separate bug for that when this is done, as it could be a good performance win.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, mozilla adaptations

This one passes tests. Let's see how it goes:

https://tbpl.mozilla.org/?tree=Try&rev=2cd19d1f321f

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, mozilla adaptations

fix android build, and fix an assert firing on tbpl. windows builds are still busted.

https://tbpl.mozilla.org/?tree=Try&rev=abae66c0a134

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, mozilla adaptations

Another go, this should fix Windows builds. There's also some M-other failures from the last push that I can't reproduce locally, will see if that happens again.

https://tbpl.mozilla.org/?tree=Try&rev=db5484d09d3e

Comment 13

[David Mandelin [:dmandelin]]

Comment on attachment 669363 [details] [diff] [review]
part 2, mozilla adaptations

Review of attachment 669363 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good. But I have a question: I see that you folded our local bug fixes (PPC, character classes, alternating groups optimization) into this patch. Do you have a separate copy for them? I think it would be useful to maintain them separately so that we can do things like easily drop one of the bug fixes if WebKit fixes it upstream.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to David Mandelin [:dmandelin] from comment #13)
> Comment on attachment 669363 [details] [diff] [review]
> part 2, mozilla adaptations
> 
> Review of attachment 669363 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Patch looks good. But I have a question: I see that you folded our local bug
> fixes (PPC, character classes, alternating groups optimization) into this
> patch. Do you have a separate copy for them? I think it would be useful to
> maintain them separately so that we can do things like easily drop one of
> the bug fixes if WebKit fixes it upstream.

I can extricate some of the syntactic ones and the alternating groups optimization, but many of the changes are just fixing things that don't compile in our build system (like CPU(X86) -> WTF_CPU_X86) which we probably couldn't upstream.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.0, syntactic tweaks

Potentially upstreamable syntactic tweaks.

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.1, offset error handling

Upstreamable patch to handle errors in the offset setup code.

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.2, consumeNumber error handling

Upstreamable patch to make consumeNumber() fallible.

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.3, don't allow broken ranges

I separated this out into a new patch, but it looks like WebKit explicitly wanted different functionality.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.4, upstreamable alternative fallback fix

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2.5, mozilla build adaptations

These changes, I think, are mostly non-upstreamable (let me know if I missed any or if we can bridge some more).

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Finally got time to look into the win64 problems. New try run:

https://tbpl.mozilla.org/?tree=Try&rev=c3fcb4ffc474

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf2f8cb5e73

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bf2f8cb5e73

Comment 24

[Philip Chee]

VS2008SP1 (VC9)
I'm getting lots of "Cannot open include file: 'stdint.h': No such file or directory"

c:\t1\hg\comm-central\mozilla\js\src\yarr\CheckedArithmetic.h(34) : fatal error C1083: Cannot open include file: 'stdint.h': No such file or directory