Bug 40935 - Tightened up some get_by_id_chain* code generation
Summary: Tightened up some get_by_id_chain* code generation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Trivial
Assignee: Nathan Lawrence
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-21 12:57 PDT by Nathan Lawrence
Modified: 2010-08-04 07:19 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.15 KB, patch)
2010-06-21 12:57 PDT, Nathan Lawrence
ggaren: review-
Details | Formatted Diff | Diff
Patch (now with code!) (9.93 KB, patch)
2010-06-21 14:32 PDT, Nathan Lawrence
no flags Details | Formatted Diff | Diff
Patch (10.73 KB, patch)
2010-06-30 11:37 PDT, Nathan Lawrence
oliver: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (10.79 KB, patch)
2010-07-29 09:31 PDT, Nathan Lawrence
oliver: review+
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nathan Lawrence 2010-06-21 12:57:10 PDT
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.
Comment 1 Nathan Lawrence 2010-06-21 12:57:37 PDT
Created attachment 59275 [details]
Patch
Comment 2 Geoffrey Garen 2010-06-21 13:12:57 PDT
Comment on attachment 59275 [details]
Patch

Looks like the patch you attached is only a ChangeLog :(.
Comment 3 Nathan Lawrence 2010-06-21 14:32:32 PDT
Created attachment 59291 [details]
Patch (now with code!)

ggaren:
You are quite correct.  I did only upload the Changelog.
Comment 4 Darin Adler 2010-06-30 10:35:13 PDT
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()?
Comment 5 Nathan Lawrence 2010-06-30 11:37:03 PDT
Created attachment 60138 [details]
Patch
Comment 6 WebKit Commit Bot 2010-07-27 18:07:50 PDT
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
Comment 7 Nathan Lawrence 2010-07-29 09:31:20 PDT
Created attachment 62961 [details]
Patch

up to date.
Comment 8 WebKit Commit Bot 2010-07-29 14:15:53 PDT
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
Comment 9 Geoffrey Garen 2010-08-03 11:07:15 PDT
What's the status of this patch? Is there a real error, or is the commit queue confused?
Comment 10 Eric Seidel (no email) 2010-08-03 11:14:36 PDT
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.
Comment 11 Eric Seidel (no email) 2010-08-04 07:15:56 PDT
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.
Comment 12 WebKit Commit Bot 2010-08-04 07:19:55 PDT
Committed r64649: <http://trac.webkit.org/changeset/64649>