Closed Bug 1333018 Opened 3 years ago Closed 2 years ago

Wasm baseline: Cleanup by-ref arguments, turn into by-pointer

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: lth, Assigned: lth, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
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.
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.
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
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 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+
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
Ok great, thank you for your help.
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
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)
Lars, did you want me to change those functions for you?
First I want Benjamin's opinion on whether they need to be changed at all :)
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)
(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.
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 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+
Status: NEW → ASSIGNED
Pushed by lhansen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a9390557aab
Clean up by-ref and by-pointer arguments.  r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/9a9390557aab
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.