1706190 - Simplify arguments to IRGenerator constructor

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P3)

Description

[Iain Ireland [:iain]]

Note: this bug is currently reserved for Outreachy applicants for the Spring/Summer 2021 cycle. If it has not been completed by the end of the application cycle, we will open it up.

Background

Inline caches (ICs) are an important technique for optimizing JS. When we execute code that has many possible behaviours (for example, a + b might be addition or string concatenation, depending on what a and b are), we call a fallback function that tries to attach an IC stub. (For example, it might check whether a and b are numbers, and immediately return their sum if they are.) The next time we execute that code, we will try the stub first, and only execute the fallback function if the stub fails. If the code is polymorphic, we may attach several stubs to the same IC chain.

There are many different kinds of IC, each of which has its own implementation of IRGenerator.

This Bug

In bug 1695396, we modified IRGenerator and its subclasses so that we know whether this is the first time we have ever executed this code. We do this by looking at the ICState and passing an isFirstStub argument into each constructor.

We were already passing a mode argument to each constructor, which also comes from the ICState. To reduce the number of arguments, we should replace the mode and isFirstStub arguments to the IRGenerator constructor with a single ICState& argument, and use it to initialize isFirstStub_ and mode_. This will make it easier to make similar changes in the future.

We should also move the implementation of newStubIsFirstStub from ICFallbackStub into ICState, to make it easier to share between baseline and Ion ICs.

To fix this bug, you will likely have to modify CacheIR.h, CacheIR.cpp, BaselineIC.cpp, IonIC.cpp, and ICState.h. Looking at the patch for bug 1695396 is a good starting point.

When you're done, you should be able to run the test suite (./mach jit-test) without any failures. (You will want to build SpiderMonkey with --enable-optimize --enable-debug while running the tests.)

Prerequisites

Before getting started, you'll want to ensure:

• you have a checkout of the Firefox source code
• you can build SpiderMonkey
• you have read this walkthrough about how development works in Firefox

Getting help

Leave comments on this bug for questions, or ask in #spidermonkey on chat.mozilla.org.

Comment 1

[mehaboob shariff]

Hello Iain,
I'd like to take up this bug.

Comment 2

[mehaboob shariff]

Just a few doubts before getting started :

1. Unlike the previous patch, now I will have to remove isFirstStub along with mode from all the constructors (also from protected member of IRGenerator) that I have passed into earlier right ?

2. But then then will be no need of isFirstStub anywhere because we directly call it from ICState by using newStubIsFirstStub

Comment 3

[mehaboob shariff]

one more thing is that can you let me know how to check for these whitespaces before submission.
I have used an extension in VScode to remove whitespaces and also checked using clang-format to remove these but still one or two white blanks remained

Comment 4

[mehaboob shariff]

Attachment: Bug 1706190: Simplify the arguments to IRGenerator constructor r=iain

Comment 5

[mehaboob shariff]

Hi Iain ,
I'd have made a submission but there is a mistake I did while trying to solve the conflicts (merged with some other patch so there are some changes in code which I haven't made and is causing an error) as sfink suggested.
I'll correct it and submit again

Comment 6

[Iain Ireland [:iain]]

I think I answered most of these questions on the Phabricator revision.

I believe clang-format should remove end-of-line whitespace, so the only other whitespace changes you have to watch out for are new/removed newlines. Skimming through the output of hg diff to see if there are any changes that don't belong should be sufficient. If one sneaks through, it's not a big deal.

Comment 7

[mehaboob shariff]

Hi Iain,
Sorry I haven't refreshed the phabricator page so I didn't see your message and made a submit.
I have just seen your approach but I already changed the code that is using mode and isFirstStub , if it a problem then let me know I'll change the code again and resubmit.
Thank you for the clarification on the merge conflict part and also noted about the convention of commit message.

Comment 8

[mehaboob shariff]

would you mind if you could point me to some other good bugs as I couldn't find one which is not occupied

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b35ee13d7aa5
Simplify the arguments to IRGenerator constructor r=iain

Comment 10

[Atila Butkovits]

Backed out for causing build bustages on BaselineIC.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/e7c3344e6ae7fd04cef466dd0efbd916a6929a45

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=JTZzWh_HQDC_II5cKW0Cgw.0&revision=b35ee13d7aa5321bdf84a47d0215ca6570589ef7&searchStr=linux%2Cx64%2Copt%2Cbuild-linux64%2Fopt%2Cb

Failure log: https://treeherder.mozilla.org/logviewer?job_id=337478505&repo=autoland&lineNumber=15567

Comment 11

[mehaboob shariff]

Is there something that needs to be done?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3261528439b
Simplify the arguments to IRGenerator constructor r=iain

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/e3261528439b