Closed
Bug 1333018
Opened 7 years ago
Closed 6 years ago
Wasm baseline: Cleanup by-ref arguments, turn into by-pointer
Categories
(Core :: JavaScript Engine: JIT, defect, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: lth, Assigned: lth, Mentored)
References
Details
Attachments
(2 files, 2 obsolete files)
16.96 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
14.13 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
A number of baseline methods have output arguments that are now passed through a reference API (eg, int32_t&) not a pointer API (eg, int32_t*), popConstI32 is an example. Since the house style is to use a pointer API here we should clean these up.
Assignee | ||
Updated•7 years ago
|
Blocks: wasm-baseline-perf
Comment 1•7 years ago
|
||
Hello, I fixed a few of the method output arguments in the attached patch, if it's ok I can try to fix the rest of them. This is my first time contributing, so I just wanted to be sure I was doing this correctly.
Assignee | ||
Comment 2•7 years ago
|
||
Hi Crispin, the fixes look all right but I suggest you stick to the ones you've done already and not make any more until you get a hang of how this works. The first thing you need to do is to attach a patch to this bug, ie, the changes you've made in a "diff" format and not as the whole file you're fixing. There are several ways to do this, but for now you can attach the output of "hg diff" or "git diff", depending on whether you're using Mercurial or Git for managing your Firefox tree. Only then is it practical for us to inspect the work you've done and provide feedback (and eventually, review), and the changes must be in that format for landing in the source tree eventually, anyway.
Comment 3•7 years ago
|
||
Hi Lars, sorry about that, this patch should show the diff. The patch changes 4 methods to be "pass by pointer" instead of "pass by reference". Thank you for your help.
Attachment #8842237 -
Attachment is obsolete: true
Comment 4•7 years ago
|
||
Hi :chb, thanks for the patch, it looks great to me. Can you change the commit message according to https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions, please? Then we'll check it in our testing continuous integration before it gets merged in Firefox. Cheers!
Comment 5•7 years ago
|
||
Hi :bbouvier. I modified the commit message for the patch, does it look ok? Thank you.
Attachment #8842735 -
Attachment is obsolete: true
Comment 6•7 years ago
|
||
Comment on attachment 8843121 [details] [diff] [review] Bug 1333018 - changed argument type from reference to pointer in popConstI32, popConstI64, popConstPositivePowerOfTwoI32, and popConstPositivePowerOfTwoI64 to adhere to the API coding style Review of attachment 8843121 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Changes look good to me. I've pushed your patch to our continuous integration (try server), and if everything goes green (building and test pass), then we can push it to our production CI. https://treeherder.mozilla.org/#/jobs?repo=try&revision=e067a8df9294f970b04b60db8c87439f481f3a34 Feel free to make patches for other places in this code where arguments are passed by reference :)
Attachment #8843121 -
Flags: review+
Updated•7 years ago
|
Keywords: checkin-needed,
leave-open
Updated•7 years ago
|
Assignee: nobody → chb2ab
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ffd6d73ab8 Change argument type from reference to pointer in popConstI32, popConstI64, popConstPositivePowerOfTwoI32, and popConstPositivePowerOfTwoI64 to adhere to the API coding style. r=bbouvier
Keywords: checkin-needed
Comment 8•7 years ago
|
||
Ok great, thank you for your help.
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c6ffd6d73ab8
Assignee | ||
Comment 10•6 years ago
|
||
I think this is probably done, but I'm waiting for some feedback re initControl, startCall, endCall, and similar, which take by-ref arguments that are initialized (some partly, some fully).
Assignee: chb2ab → lhansen
Assignee | ||
Comment 11•6 years ago
|
||
From IRC (edited & augmented): i'm wondering where to draw the line here. Consider the method initControl() in WasmBaselineCompiler.cpp it takes a `Control& item` the item in question has been default-initialized with sentinel values but here we basically set all fields one after the other so should this be a by-pointer or by-ref signature? Similarly for startCall, endCall, and probably others.
Flags: needinfo?(bbouvier)
Comment 12•6 years ago
|
||
Lars, did you want me to change those functions for you?
Assignee | ||
Comment 13•6 years ago
|
||
First I want Benjamin's opinion on whether they need to be changed at all :)
Comment 14•6 years ago
|
||
endCall should take const&, since it doesn't modify the FunctionCall argument. For others, the logic is the following: we want to be able to see at *call sites* that something is getting modified by the function call. In this case, beginCall's FunctionCall is modified, as well as initControl's Control. For instance, for the latter, there's no ultra-obvious way to tell that the result of controlItem() was default-initialized; that implies more cognitive load when reading the code, because then you need to track down that the element was default initialized in the first place. And it's not obvious the passed argument is modified, although when you see the & (taking the address) operator, it is obvious that something is going on. My rule of thumb for this, which I copied from Luke's code mainly, is the following: - use const& when passing a read-only entity - use pointer when passing a read-write ("in-out") argument Most of the code under wasm is following this logic, I think. It's all good to use local references within methods to avoid using the -> operator, if that's really more convenient and justified, like this: Some& something = *aSomething; Does it make sense?
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #14) > For others, the logic is the following: we want to be able to see at *call > sites* that something is getting modified by the function call. > ... > Does it make sense? Let's just say I don't mind implementing it. > My rule of thumb for this, which I copied from Luke's code mainly, is the > following: > - use const& when passing a read-only entity > - use pointer when passing a read-write ("in-out") argument Sure, easy to remember. I'm generally on the fence about rules like these since they tend to break abstraction boundaries - you expose internal side effects. (Then to counter that you end up with eg 'mutable'.) This is a bit like exception annotations in Java; yes, they make sense and clarify the APIs, but gosh, you end up talking about a lot of things you really don't need to be talking about all the time. But I'm probably happiest if we all get along, so I'll gladly accept (or write) a patch that cleans up what's left.
Assignee | ||
Comment 16•6 years ago
|
||
Clean up many more arguments by turning them into const T& or T* as appropriate. The main non-const references left after this are MacroAssembler& and BaseRegAlloc&. These I will probably not convert, but in any case the changes in this patch don't have to block on that discussion.
Attachment #8958415 -
Flags: review?(bbouvier)
Comment 17•6 years ago
|
||
Comment on attachment 8958415 [details] [diff] [review] bug1333018-clean-up-by-ref-arguments.patch Review of attachment 8958415 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, thanks. ::: js/src/wasm/WasmBaselineCompile.cpp @@ +2248,4 @@ > moveI32(src.i32reg(), dest); > } > > + void loadConstI64(const Stk &src, RegI64 dest) { nit: & next to Stk @@ +2264,4 @@ > moveI64(src.i64reg(), dest); > } > > + void loadConstF64(const Stk &src, RegF64 dest) { ditto @@ +2282,4 @@ > moveF64(src.f64reg(), dest); > } > > + void loadConstF32(const Stk &src, RegF32 dest) { ditto @@ +3397,4 @@ > return AlignBytes(i.stackBytesConsumedSoFar(), 16u); > } > > + void startCallArgs(FunctionCall* call, size_t stackArgAreaSize) light suggestion: put inout args in last parameter positions? @@ +3430,4 @@ > // we have the outgoing size at low cost, and then we can pass > // args based on the info we read. > > + void passArg(FunctionCall* call, ValType type, const Stk& arg) { ditto
Attachment #8958415 -
Flags: review?(bbouvier) → review+
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Keywords: good-first-bug,
leave-open
Comment 18•6 years ago
|
||
Pushed by lhansen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a9390557aab Clean up by-ref and by-pointer arguments. r=bbouvier
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a9390557aab
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•