1538557 - Use BytecodeLocation and BytecodeIterator in JitScript::initICEntriesAndBytecodeTypeMap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Background

Historically access to bytecode in SpiderMonkey has been fairly freeform and common. This means that common idioms are repeated without encapsulation, and it's difficult to audit the codebase for patterns which makes changes around bytecode much more fragile than we'd like.

We'd like to encapsulate manipulation of bytecode within the engine to a set of accessor classes where possible.

This Bug

By using the BytecodeLocation and BytecodeIterator, we'd like to replace uses of jsbytecode* and pcOffset inside ICScript::create.

This bug is only for changing the implementation of this function: It's OK that when values escape this function the return to jsbytecode* and pcOffsets. We can't change everything at once!

Note: It is expected that you may have to expand the interface of BytecodeLocation or BytecodeIterator. The interfaces as they exist now are driven by clients, and are comparatively thin.

Prior Art

• In Bug 1499544, Part 2 used these interfaces to modify JSScript::assertValidJumpTargets. You can use that patch as inspiration for what we're looking for.

Prerequisites

Before getting started, you'll want to

• You Have a checkout of the Firefox source code
• Make sure you can build SpiderMonkey
• Read this walkthrough about how development works in Firefox

Getting Help

Feel free to leave comments on this bug for questions, or, if you have more synchronous questions about this bug, feel free to drop into #jsapi on irc.mozilla.org.

Tips:

• Not sure if the code you've been editing is getting run? Insert a call to MOZ_CRASH, a macro which will crash when executed, and run the entire test suite with an optimized build (for speed). If you see crashes, you can then use a debug build to make sure it's crashing in your code!

Comment 1

[Jan de Mooij [:jandem]]

Please hold off on this for a bit, I'm making some changes to this code and it would be good to avoid merge conflicts :)

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #1)

Please hold off on this for a bit, I'm making some changes to this code and it would be good to avoid merge conflicts :)

We could do this after bug 1551796 lands...

Comment 3

[chirag]

Hey I would like to contribute and volunteer to fix this bug. I am just a beginner and starting off my open source journey so please help me.

Comment 4

[Adam Holm]

I had just started working on this, but let's see what @Matt thinks.

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Hey Chirag, I've opened Bug 1566057 and CC'd you on it. It's very similar to this one, but at a different location so we don't overlap your work and Adam's work, who has already started looking at this one.

Comment 6

[Adam Holm]

Attachment: Bug 1538557 - Use BytecodeLocation and BytecodeIterator in JitScript::initICEntriesAndBytecodeTypeMap to replace uses of jsbytecode* and pcToOffset. r=mgaudet

Comment 7

[Adam Holm]

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bda190eeb8391331675895fd974b295cfa5e6fe

Comment 8

[Adam Holm]

(In reply to Adam Holm from comment #7)

Try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bda190eeb8391331675895fd974b295cfa5e6fe

Try build with the error behind the failures in this build (hopefully) fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1aec7933967e6a7ac07b7e62662fc0141c9b648

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce02f51dbf78
Use BytecodeLocation and BytecodeIterator in JitScript::initICEntriesAndBytecodeTypeMap to replace uses of jsbytecode* and pcToOffset. r=jandem,tcampbell

Comment 10

[Razvan Maries]

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