Use BytecodeLocation and BytecodeIterator in GetPCCountScriptSummary
Categories
(Core :: JavaScript Engine, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox83 | --- | fixed |
People
(Reporter: asorholm, Assigned: yozaam)
References
Details
(Whiteboard: [lang=c++])
Attachments
(1 file)
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 GetPCCountScriptSummary
.
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 pcOffset
s. 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. -
Dependencies of Bug 1478034 that have been marked
RESOLVED FIXED
can also be used 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
- Understand the basics of using Mercurial
- Understand how to submit a patch
- Read this walkthrough about how development works in Firefox
This patch is done when
-
Direct access to the bytecode in
GetPCCountScriptSummary
is abstracted through the use of the classesBytecodeLocation
andBytecodeIterator
. In other words,jsbytecode*
andoffsetToPC
uses inGetPCCountScriptSummary
are replaced with methods and members of the classesBytecodeLocation
andBytecodeIterator
. -
Your patch passes the test suites described here.
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•6 years ago
|
||
I'm going to drop the good-first-bug
keyword, just because it looks like there's a browser consumer of this, and it's not 100% clear what test-cases we can run to make sure the changes are correct here.
While not impossible, it's just more complicated.
(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #1)
I'm going to drop the
good-first-bug
keyword, just because it looks like there's a browser consumer of this, and it's not 100% clear what test-cases we can run to make sure the changes are correct here.While not impossible, it's just more complicated.
Sounds good to me :)
Comment 3•6 years ago
|
||
(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #1)
[…] it's not 100% clear what test-cases we can run to make sure the changes are correct here.
GetPCCountScriptSummary
is being tested by https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/profiler/pc-count-profiler.js#25-26
Comment 4•6 years ago
|
||
Sweet! That's great to know.
Updated•6 years ago
|
https://searchfox.org/mozilla-central/source/js/src/vm/BytecodeUtil.cpp#2678
^^ RootedScript -> does this need to change ?
Or just the for loop ->
jsbytecode* codeEnd = script->codeEnd();
for (jsbytecode* pc = script->code(); pc < codeEnd; pc = GetNextPc(pc)) {
if (const PCCounts* counts = sac.maybeGetPCCounts(pc)) {
total += counts->numExec();
}
}
What about maybeGetPCCounts(pc) will this change or I will just pass BytecodeLocation::toRawBytecode() instead of pc ?
Comment 6•5 years ago
|
||
Shouldn't need to change that no; for this bug, just the loop suffices.
An aside: it's easy for questions like this to accidentally get lost. To try to avoid that, below the comment field in Bugzilla there's this checkbox called "Request Information from"... into which you can put my email. If you do that, it'll show up as a notification on my account to remind me to get to this.
Updated•5 years ago
|
Comment 10•5 years ago
|
||
bugherder |
Description
•