Bug 37684

Summary: [Qt] JSValue QtClass::fallbackObject can be optimized
Product: WebKit Reporter: Anders Bakken <agbakken>
Component: WebKit QtAssignee: Anders Bakken <agbakken>
Status: CLOSED FIXED    
Severity: Enhancement CC: commit-queue, hausmann, jesus, kenneth, kimmo.t.kinnunen
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
patch that reduces memcpy'ing
none
Patch
commit-queue: commit-queue-
New version of the patch none

Description Anders Bakken 2010-04-15 16:58:12 PDT
Created attachment 53489 [details]
patch that reduces memcpy'ing

JSValue QtClass::fallbackObject(ExecState* exec, Instance* inst, const Identifier& identifier) copies and modifies more string data than it needs to.

The attached patch improves this.
Comment 1 Jesus Sanchez-Palencia 2010-05-11 14:59:34 PDT
(In reply to comment #0)
> The attached patch improves this.

It is necessary to mention this bug report on the Changelog. Also, do you want it to be reviewed?
If so you have to change it to r? .


Kenneth, can you take a look at this? LGTM. ;)
Comment 2 Kenneth Rohde Christiansen 2010-05-11 15:02:41 PDT
I guess you wanted to put this up for review
Comment 3 Anders Bakken 2010-05-12 02:58:07 PDT
I did want to put it up for review. Sorry about that :-)
Comment 4 Kenneth Rohde Christiansen 2010-05-12 05:39:26 PDT
Comment on attachment 53489 [details]
patch that reduces memcpy'ing

Looks good Anders!
Comment 5 Jesus Sanchez-Palencia 2010-05-12 06:15:27 PDT
Comment on attachment 53489 [details]
patch that reduces memcpy'ing

I'll add the bug number to the changelog and upload a new patch. Just following the guidelines... ;)
Comment 6 Jesus Sanchez-Palencia 2010-05-12 06:24:45 PDT
Created attachment 55837 [details]
Patch

Added the bug number to the Changelog.
Comment 7 Jesus Sanchez-Palencia 2010-05-12 06:25:19 PDT
Comment on attachment 53489 [details]
patch that reduces memcpy'ing

Clearing flags of old patch.
Comment 8 WebKit Commit Bot 2010-05-14 00:56:13 PDT
Comment on attachment 55837 [details]
Patch

Rejecting patch 55837 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 2
Parsed 2 diffs from patch file(s).
patching file WebCore/ChangeLog
patch: **** malformed patch at line 18:  2010-04-15  Adam Roben  <aroben@apple.com>

patching file WebCore/bridge/qt/qt_class.cpp

Full output: http://webkit-commit-queue.appspot.com/results/2315058
Comment 9 Anders Bakken 2010-05-14 03:19:44 PDT
Created attachment 56062 [details]
New version of the patch
Comment 10 WebKit Commit Bot 2010-05-15 00:51:26 PDT
Comment on attachment 56062 [details]
New version of the patch

Rejecting patch 56062 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing/iframes', '--quiet']" exit_code: 1
Last 500 characters of output:
ng Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 18341 test cases.
fast/loader/recursive-before-unload-crash.html -> failed

Exiting early after 1 failures. 13417 tests run.
198.65s total testing time

13416 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/2265108
Comment 11 Kenneth Rohde Christiansen 2010-05-15 12:32:49 PDT
Anders, you cannot remove the "Reviewed by NOBODY (OOPS!!)". If you do that we cannot land using the commit-queue. It has to be exactly like the prepare-ChangeLog scripts writes it.

Now you can reupload the patch with this fixed and reset r? and cq? 

-- or --

You can upload the patch with "Reviewed by Kenneth Rohde Christiansen" and just set cq?

The latter is preferred as anyone with committer-rights can change the cq? to cq+

Another option would be to get someone to land it manually using webkit-patch or similar.
Comment 12 Adam Barth 2010-05-15 17:55:32 PDT
Comment on attachment 56062 [details]
New version of the patch

I think you were just bit by a flaky test.
Comment 13 WebKit Commit Bot 2010-05-15 19:52:32 PDT
Comment on attachment 56062 [details]
New version of the patch

Clearing flags on attachment: 56062

Committed r59563: <http://trac.webkit.org/changeset/59563>
Comment 14 WebKit Commit Bot 2010-05-15 19:52:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Simon Hausmann 2010-05-16 00:58:58 PDT
Revision r59563 cherry-picked into qtwebkit-2.0 with commit 4bb49bb59d4f763069ae6bb84b9648d6ed0bfd1d
Comment 16 Kimmo Kinnunen 2010-05-17 05:25:35 PDT
This commit causes a bug which prevents signals from being matched.

It introduces second local variable named "index", which then overrides the proper method number variable (also called "index")

this causes signals from being found in some cases.

The affected line is:
QtRuntimeMetaMethod* val = new (exec) QtRuntimeMetaMethod(exec, identifier, static_cast<QtInstance*>(inst), index, normal, false);
Comment 17 Kenneth Rohde Christiansen 2010-05-17 05:32:04 PDT
Uh, I wasn't able to see that from the diff. I will fix it later today
Comment 18 Kenneth Rohde Christiansen 2010-05-17 05:40:50 PDT
Patch shadowed a variable.
Comment 19 Kenneth Rohde Christiansen 2010-05-17 06:50:28 PDT
Fixed regression in r59606
Comment 20 Anders Bakken 2010-05-17 07:37:32 PDT
Oops. My bad.
Comment 21 Simon Hausmann 2010-05-18 00:29:30 PDT
<cherr-pick-for-backport: r59606>
Comment 22 Simon Hausmann 2010-05-18 00:30:44 PDT
<cherry-pick-for-backport: r59606>
Comment 23 Simon Hausmann 2010-05-18 00:33:41 PDT
Revision r59606 cherry-picked into qtwebkit-2.0 with commit 807157e42add842605ec67d9363dd3f1861748ca