Bug 40935 – Tightened up some get_by_id_chain* code generation

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Nathan Lawrence]

Tightened up some get_by_id_chain* code generation in the style of https://bugs.webkit.org/show_bug.cgi?id=30539, and changed code to call accessor functions when it was not necessary to directly access the private variables.

[Nathan Lawrence]

Created attachment 59275 [details]
Patch

[Geoffrey Garen]

Comment on attachment 59275 [details]
Patch

Looks like the patch you attached is only a ChangeLog :(.

[Nathan Lawrence]

Created attachment 59291 [details]
Patch (now with code!)

ggaren:
You are quite correct.  I did only upload the Changelog.

[Darin Adler]

Comment on attachment 59291 [details]
Patch (now with code!)

Looks good to me, but I should probably let Geoff, Gavin, or Oliver review. I do have a couple of comments.

> -void JIT::testPrototype(Structure* structure, JumpList& failureCases)
> +void JIT::testPrototype(JSValue proto, JumpList& failureCases)

Please use whole words, not abbreviations, in new code. So here it would be prototype, not proto.

> +#if CPU(X86_64)
> +    move(ImmPtr(asCell(proto)->structure()), regT3);
> +    failureCases.append(branchPtr(NotEqual, AbsoluteAddress(&asCell(proto)->m_structure), regT3));
> +#else
> +    failureCases.append(branchPtr(NotEqual, AbsoluteAddress(&asCell(proto)->m_structure), ImmPtr(asCell(proto)->structure())));
> +#endif

Nothing here makes it clear why X86_64 is an exception. Is this a more efficient code path? If so, could it be done for other platforms that have additional registers? Why doesn't the branchPtr function do this automatically? I'm sure there's a reason but I'd like to see a comment in the code, unless it's obvious to everyone else, and just not me.

I know there was code elsewhere that you moved here that did the same thing without a comment, so it's really someone else I should be complaining to.

> -        testPrototype(it->get(), failureCases);
> +        testPrototype(it->get()->storedPrototype(), failureCases);

Is there a way to do this without calling get()?

[Nathan Lawrence]

Created attachment 60138 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 60138 [details]
Patch

Rejecting patch 60138 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Oliver Hunt', u'--force']" exit_code: 1
Last 500 characters of output:
unk #3 succeeded at 981 (offset 3 lines).
Hunk #4 succeeded at 1051 (offset 3 lines).
1 out of 4 hunks FAILED -- saving rejects to file JavaScriptCore/jit/JITPropertyAccess32_64.cpp.rej
patching file JavaScriptCore/jit/JITPropertyAccess.cpp
Hunk #1 succeeded at 581 with fuzz 1 (offset 1 line).
Hunk #2 FAILED at 604.
Hunk #3 succeeded at 969 (offset 2 lines).
Hunk #4 succeeded at 1039 (offset 2 lines).
1 out of 4 hunks FAILED -- saving rejects to file JavaScriptCore/jit/JITPropertyAccess.cpp.rej

Full output: http://queues.webkit.org/results/3573542

[Nathan Lawrence]

Created attachment 62961 [details]
Patch

up to date.

[WebKit Commit Bot]

Comment on attachment 62961 [details]
Patch

Rejecting patch 62961 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 20740 test cases.
svg/dom/SVGScriptElement/script-set-href.svg -> failed

Exiting early after 1 failures. 18296 tests run.
514.43s total testing time

18295 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
69 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/3616364

[Geoffrey Garen]

What's the status of this patch? Is there a real error, or is the commit queue confused?

[Eric Seidel (no email)]

Comment on attachment 62961 [details]
Patch

If you believe it was a flaky test, then we should just re-queue it.

The commit-queue is *way* behind right now because leopard was broken all day yesterday and all through the night... in fact, I think the leopard build is still broken.

[Eric Seidel (no email)]

Comment on attachment 62961 [details]
Patch

Your ChangeLog is missing the bug line. :(

I'm landing this by hand using the commit-queue account (to fix a bug in the cq), but please remember the bug line in the future.  webkit-patch upload or prepare-ChangeLog --bug 12345 will generate the proper changelog entry.

[WebKit Commit Bot]

Committed r64649: <http://trac.webkit.org/changeset/64649>