Last Comment Bug 730859 - IonMonkey: JSOP*PROP and JSOP*NAME ops should use PropertyName instead of JSAtom
: IonMonkey: JSOP*PROP and JSOP*NAME ops should use PropertyName instead of JSAtom
Status: RESOLVED FIXED
[lang=c++][mentor=jandem][ion:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: James Kitchener (:jkitch)
:
Mentors:
Depends on:
Blocks: IonMonkey
  Show dependency treegraph
 
Reported: 2012-02-27 09:46 PST by Jan de Mooij [:jandem]
Modified: 2012-09-17 12:23 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace JSAtom with PropertyName in JSOP_DELPROP (4.44 KB, patch)
2012-09-15 04:11 PDT, James Kitchener (:jkitch)
no flags Details | Diff | Review
Replace JSAtom with PropertyName in JSOP_DELPROP (4.44 KB, patch)
2012-09-16 02:15 PDT, James Kitchener (:jkitch)
jdemooij: review+
Details | Diff | Review

Description Jan de Mooij [:jandem] 2012-02-27 09:46:09 PST
Filing this bug for the IonMonkey part of Waldo's patch in bug 713183.

(This may also be a good first bug.)
Comment 1 James Kitchener (:jkitch) 2012-09-13 18:16:39 PDT
(In reply to Jan de Mooij [:jandem] from comment #0)
> Filing this bug for the IonMonkey part of Waldo's patch in bug 713183.
> 
> (This may also be a good first bug.)

I would like to work on this as my first bug.
Comment 2 Jan de Mooij [:jandem] 2012-09-14 05:37:29 PDT
(In reply to James Kitchener from comment #1)
> 
> I would like to work on this as my first bug.

Awesome! I assigned this bug to you.

Sean pointed out on IRC that only JSOP_DELPROP (MDeleteProperty) has not yet been updated. If you compare MDeleteProperty and MCallGetProperty you will notice that the former has a JSAtom * member and the latter has a CompilerRootPropertyName/HandlePropertyName. We should also rename the "atom()" method to "name()".

You will also have to update IonBuilder::jsop_delprop to take a HandlePropertyName, just like IonBuilder::jsop_getprop.

See this page for information on how to build the SpiderMonkey shell:
https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey

Let me know if you have any questions, or ask in #jsapi on IRC.
Comment 3 James Kitchener (:jkitch) 2012-09-15 04:11:53 PDT
Created attachment 661482 [details] [diff] [review]
Replace JSAtom with PropertyName in JSOP_DELPROP

The changes have been made.

It passes the tests mentioned in the New to Spidermonkey page.  Are there any others I should run?

(Well it passes except for 19 ecma\Date tests and js1_5\extensions\toLocaleFormat-01.js which also failed without my modifications)
Comment 4 James Kitchener (:jkitch) 2012-09-16 02:15:23 PDT
Created attachment 661572 [details] [diff] [review]
Replace JSAtom with PropertyName in JSOP_DELPROP

fix whitespace and flagging for review
Comment 5 Jan de Mooij [:jandem] 2012-09-17 07:53:36 PDT
Comment on attachment 661572 [details] [diff] [review]
Replace JSAtom with PropertyName in JSOP_DELPROP

Review of attachment 661572 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, thanks! I will land the patch right now.

These test failures are expected, they fail when you are in a time zone other than PDT.

For IonMonkey development it's good to run jit-tests with the --ion flag:

jit-test/jit_test.py --ion $build/js

This runs every test twice: once with --ion-eager and once with --no-jm. By default, IonMonkey compiles only really hot functions (> ~10000 iterations/calls) and these flags lower that threshold to 0 and 40 respectively. This greatly improves test coverage. We should update the "New to SpiderMonkey" page and maybe change the jit-test harness to run tests with --ion-eager by default, I will file a new bug for that.
Comment 6 Jan de Mooij [:jandem] 2012-09-17 08:35:48 PDT
Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d18e2101417

Thanks again!
Comment 7 Ed Morley [:emorley] 2012-09-17 12:23:22 PDT
https://hg.mozilla.org/mozilla-central/rev/2d18e2101417

Note You need to log in before you can comment on or make changes to this bug.