Closed
Bug 1460883
Opened 6 years ago
Closed 6 years ago
Remove flow sensitive alias analysis option
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: mgaudet, Assigned: Himanshuteli, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file, 1 obsolete file)
1.53 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
In bug 1455280 flow sensitive alias analysis was removed, but the option was left behind to simplify fuzzing and bisection. Nevertheless, at some point the option should go away.
Reporter | ||
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•6 years ago
|
||
I would like to work on this can you guide me
Reporter | ||
Comment 2•6 years ago
|
||
Hey, I'd love to guide you. First step: make sure you get a copy of the sources, and are able to build at least spidermonkey [1]. In my experience, using bootstrap.py is the easiest way to get ready to go [2]. So the option used to be 'ion-aa'. A very useful tool for you is https://searchfox.org/, which will help you find the references to that option. If I recall correctly there are two of them. You'll just need to prepare a patch to excise them. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation [2]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation
Assignee | ||
Comment 3•6 years ago
|
||
I have successfully build firefox and compiled it. If I'm correct, we need to remove the lines: if (op.getStringOption("ion-aa")) {} || !op.addStringOption('\0', "ion-aa", "flow-sensitive/flow-insensitive", "Specify wheter or not to use flow sensitive Alias Analysis" "(default: flow-insensitive)") located in js/src/shell/js.cpp Let's wait for more info.
Reporter | ||
Comment 4•6 years ago
|
||
Those look about right to me. So now, you'll do the removal, ensure things still build, then upload a patch to this bug.
Assignee | ||
Comment 5•6 years ago
|
||
Removed flow sensitive alias analysis option
(In reply to Himanshuteli from comment #5) > Created attachment 8975864 [details] [diff] [review] > js.cpp > > Removed flow sensitive alias analysis option Please join #introduction on irc.mozilla.org, then I will help you create a patch.
Otherwise, this maybe helps: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee | ||
Comment 8•6 years ago
|
||
Removed flow sensitive alias analysis option. I hope this patch is correct.
Attachment #8975864 -
Attachment is obsolete: true
Updated•6 years ago
|
Assignee: nobody → Himanshuteli
Comment 9•6 years ago
|
||
Comment on attachment 8979027 [details] [diff] [review] 1460883-Remove-flow-sensitive-alias-analysis-option Review of attachment 8979027 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this! :)
Attachment #8979027 -
Flags: review+
Comment 10•6 years ago
|
||
If you are happy with the patch, add "checkin-needed" to the |Keywords| list (which you can edit by clicking Edit Bug near the top of page). A Sheriff will add my name to the commit message and checkin the patch for us.
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 11•6 years ago
|
||
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5184707be08 Remove flow sensitive alias analysis option r=tcampbell
Keywords: checkin-needed
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f5184707be08
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Comment 13•6 years ago
|
||
\o/ Congrats on landing your first SpiderMonkey patch!
You need to log in
before you can comment on or make changes to this bug.
Description
•