Bug 21200 - Allow direct access to 'arguments' without using op_resolve
Summary: Allow direct access to 'arguments' without using op_resolve
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Cameron Zwarich (cpst)
URL:
Keywords:
Depends on:
Blocks: 20813
  Show dependency treegraph
 
Reported: 2008-09-28 11:56 PDT by Cameron Zwarich (cpst)
Modified: 2008-09-28 20:04 PDT (History)
2 users (show)

See Also:


Attachments
Patch in progress (33.43 KB, patch)
2008-09-28 14:30 PDT, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (33.68 KB, patch)
2008-09-28 19:14 PDT, Cameron Zwarich (cpst)
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-09-28 11:56:38 PDT
Currently, we access 'arguments' via op_resolve on the JSActivation. This is a decent performance hit on the V8 Raytrace benchmark. I have a patch to put the arguments object in the callframe and access it directly. This does mean using an extra callframe slot, but a slot will be freed up after fixing bug 21123. Hopefully it won't be a performance regression in the meantime.

I have a few more little things to fix up with my patch and then I'll post it.
Comment 1 Cameron Zwarich (cpst) 2008-09-28 14:30:31 PDT
Created attachment 23897 [details]
Patch in progress

Here is a patch in progress. I only perf tested non-CTI, and while SunSpider is fine, the V8 results are bizarre:

TEST              COMPARISON            FROM                 TO             DETAILS

=============================================================================

** TOTAL **:      *1.029x as slow*  5983.8ms +/- 0.2%   6156.1ms +/- 0.1%     significant

=============================================================================

  v8:             *1.029x as slow*  5983.8ms +/- 0.2%   6156.1ms +/- 0.1%     significant
    crypto:       *1.114x as slow*   999.4ms +/- 0.1%   1113.1ms +/- 0.1%     significant
    deltablue:    *1.023x as slow*  1766.6ms +/- 0.1%   1807.7ms +/- 0.1%     significant
    earley-boyer: *1.013x as slow*   678.5ms +/- 0.1%    687.2ms +/- 0.1%     significant
    raytrace:     1.030x as fast     657.6ms +/- 0.1%    638.8ms +/- 0.1%     significant
    richards:     *1.015x as slow*  1881.7ms +/- 0.6%   1909.4ms +/- 0.1%     significant
Comment 2 Cameron Zwarich (cpst) 2008-09-28 19:14:45 PDT
Created attachment 23899 [details]
Proposed patch

Maciej says he does not see any of the bizarre regressions that I do, so he is ready to review this.
Comment 3 Maciej Stachowiak 2008-09-28 19:45:35 PDT
Comment on attachment 23899 [details]
Proposed patch

r=me

Some optional comments:

- consider renaming create_arguments to init_arguments (perhaps init should be renamed to enter)

- in the parser, instead of comparing to "arguments" consider comparing to  GOBAL_DATA->propertyNames.arguments()
Comment 4 Maciej Stachowiak 2008-09-28 19:59:07 PDT
I sadly duplicated the logic for UString::from(int) and UString::from(double) though fortunately not the actual append logic. Perhaps I should try harder to refactor to make it ot duplicat, though it was not 100% clear how to do this except maybe using a template. UString::from(int) is not much code but the double case is nontrivial.
Comment 5 Maciej Stachowiak 2008-09-28 20:00:56 PDT
Oops, that comment was meant for but 21203.
Comment 6 Cameron Zwarich (cpst) 2008-09-28 20:04:34 PDT
Landed in r37050.