Summary: | Tightened up some get_by_id_chain* code generation | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nathan Lawrence <nlawrence> | ||||||||||
Component: | JavaScriptCore | Assignee: | Nathan Lawrence <nlawrence> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Trivial | CC: | commit-queue, ggaren, nlawrence, slewis | ||||||||||
Priority: | P4 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Nathan Lawrence
2010-06-21 12:57:10 PDT
Created attachment 59275 [details]
Patch
Comment on attachment 59275 [details]
Patch
Looks like the patch you attached is only a ChangeLog :(.
Created attachment 59291 [details]
Patch (now with code!)
ggaren:
You are quite correct. I did only upload the Changelog.
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()? Created attachment 60138 [details]
Patch
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 Created attachment 62961 [details]
Patch
up to date.
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 What's the status of this patch? Is there a real error, or is the commit queue confused? 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 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. Committed r64649: <http://trac.webkit.org/changeset/64649> |