Closed Bug 1438842 Opened 2 years ago Closed 2 years ago

Remove unused parameters in js/src

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: anba, Assigned: anba)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 5 obsolete files)

Remove unused parameters reported by "misc-unused-parameters" clang-tidy check.
I am not sure we always want that!

In some cases we do have unused parameters on purpose, such as the AutoSaveLiveRegisters argument of icBuildOOLFakeExitFrame [1]. This API ignores its argument on purpose, in order to enforce developers to create an AutoSaveLiveRegisters instance with icSaveLive function.

[1] https://searchfox.org/mozilla-central/source/js/src/jit/MacroAssembler.h#2605,2609
Attached patch bug1438842-part1-gc.patch (obsolete) — Splinter Review
This patch removes unused parameters from js/src/gc and js/src/jsgc.cpp as reported by clang-tidy.

If some of these unused parameters are intentionally unused, e.g. for symmetry with other functions, I can restore them.
Attachment #8951607 - Flags: review?(jcoppeard)
Attached patch bug1438842-part2-other.patch (obsolete) — Splinter Review
This patch removes unused parameters from js/src/builtin, js/src/frontend, and js/src/*.cpp as reported by clang-tidy.

If some of these unused parameters are intentionally unused, e.g. for symmetry with other functions, I can restore them.
Attachment #8951608 - Flags: review?(jorendorff)
Attached patch bug1438842-part3-jit.patch (obsolete) — Splinter Review
This patch removes unused parameters from js/src/jit as reported by clang-tidy.

As written above, if some of the unused parameters are intentionally unused, I can restore them.
Attachment #8951610 - Flags: review?(tcampbell)
Attached patch bug1438842-part4-vm.patch (obsolete) — Splinter Review
This patch removes unused parameters from js/src/vm as reported by clang-tidy.

As written above, if some of the unused parameters are intentionally unused, I can restore them.
Attachment #8951611 - Flags: review?(jdemooij)
Attached patch bug1438842-part5-wasm.patch (obsolete) — Splinter Review
This patch removes unused parameters from js/src/jit as reported by clang-tidy.

As written above, if some of the unused parameters are intentionally unused, I can restore them.

The "errorNumber" parameter of |CompileStreamTask::rejectAndDestroyAfterHelperThreadStarted(...)| was unused, but I assume it should be used for the streamError_ field.
Attachment #8951613 - Flags: review?(bbouvier)
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> I am not sure we always want that!

Yes, I didn't expect anything else. :-)

Nevertheless I thought it'd make sense to see if any of these unused parameters can actually be removed, for example if we missed to remove them when refactoring code, or to check if these unused parameters should actually be used and not using them is a bug in the current code. 

> In some cases we do have unused parameters on purpose, such as the
> AutoSaveLiveRegisters argument of icBuildOOLFakeExitFrame [1]. This API
> ignores its argument on purpose, in order to enforce developers to create an
> AutoSaveLiveRegisters instance with icSaveLive function.

I didn't touch any functions with "AutoSomething" parameters, for example GC code has many AutoLockGC parameters.
Comment on attachment 8951607 [details] [diff] [review]
bug1438842-part1-gc.patch

Review of attachment 8951607 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thanks for doing this!

::: js/src/gc/GCInternals.h
@@ -59,5 @@
>  {
>      mozilla::Maybe<AutoTraceSession> session_;
>  
>    public:
> -    AutoPrepareForTracing(JSContext* cx, ZoneSelector selector);

I removed this one yesterday :)
Attachment #8951607 - Flags: review?(jcoppeard) → review+
Comment on attachment 8951613 [details] [diff] [review]
bug1438842-part5-wasm.patch

Review of attachment 8951613 [details] [diff] [review]:
-----------------------------------------------------------------

That's really sweet, thanks.

::: js/src/wasm/WasmJS.cpp
@@ +2185,1 @@
>          Handle<PromiseObject*> promise, bool instantiate, HandleObject importObj)

nit: can probably reorganize line to fit more arguments on the first line

@@ +2497,5 @@
>      // See setClosedAndDestroyAfterHelperThreadStarted() comment.
>      bool rejectAndDestroyAfterHelperThreadStarted(unsigned errorNumber) {
>          MOZ_ASSERT(streamState_.lock() == Code || streamState_.lock() == Tail);
>          MOZ_ASSERT(!streamError_);
> +        streamError_ = Some(errorNumber);

Nice catch.
Attachment #8951613 - Flags: review?(bbouvier) → review+
Comment on attachment 8951611 [details] [diff] [review]
bug1438842-part4-vm.patch

Review of attachment 8951611 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent work, thanks!

::: js/src/vm/NativeObject.cpp
@@ +2272,5 @@
>   *     web, as there are too many false positives, but anecdotally useful in
>   *     Gecko code.)
>   */
>  static bool
> +GetNonexistentProperty(JSContext* cx, HandleId id, IsNameLookup nameLookup, MutableHandleValue vp)

Great change.
Attachment #8951611 - Flags: review?(jdemooij) → review+
Comment on attachment 8951610 [details] [diff] [review]
bug1438842-part3-jit.patch

Review of attachment 8951610 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

One 'script' var looks still in use. A couple cases I'd prefer to keep. I've also tagged a few follows for someone to cleanup since I was reading through all this code.

::: js/src/jit/BaselineBailouts.cpp
@@ +1665,5 @@
>          bool passExcInfo = handleException || propagatingExceptionForDebugMode;
>  
>          jsbytecode* callPC = nullptr;
>          RootedFunction nextCallee(cx, nullptr);
> +        if (!InitFromBailout(cx, callerPC, fun, scr,

Follow-up: we should remove the local caller, callerPC variables and pass frameNo instead.

::: js/src/jit/BaselineCompiler.cpp
@@ +233,5 @@
>      baselineScript->copyPCMappingEntries(pcEntries);
>  
>      // Copy IC entries
>      if (icEntries_.length())
> +        baselineScript->copyICEntries(script, &icEntries_[0]);

Follow-up: CodeLocationLabel::repoint should be fixed so IonScript::copyICEntries can remove the masm arg as well.

@@ +4635,5 @@
>      pushArg(R0.scratchReg());
>      return callVM(DebugAfterYieldInfo);
>  }
>  
> +typedef bool (*FinalSuspendFn)(JSContext*, HandleObject, jsbytecode*);

\o/

::: js/src/jit/BaselineDebugModeOSR.cpp
@@ +267,5 @@
>      return true;
>  }
>  
>  static bool
> +CollectInterpreterStackScripts(const Debugger::ExecutionObservableSet& obs,

Slight preference to keeping.

::: js/src/jit/BaselineInspector.cpp
@@ +755,5 @@
>      return true;
>  }
>  
>  static bool
> +AddCacheIRGlobalGetter(ICCacheIR_Monitored* stub,

Leave this.

Follow-up: |if (innerized) return false;|

::: js/src/jit/BaselineJIT.cpp
@@ +949,5 @@
>  }
>  
>  #ifdef JS_TRACE_LOGGING
>  void
> +BaselineScript::initTraceLogger(JSScript* script, const Vector<CodeOffset>& offsets)

This still looks used:
https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/js/src/jit/BaselineJIT.cpp#966

::: js/src/jit/IonBuilder.cpp
@@ +7556,5 @@
> +    return jsop_getname();
> +}
> +
> +AbortReasonOr<Ok>
> +IonBuilder::jsop_getname()

Keep this for consistency with opcode definition.

::: js/src/jit/MCallOptimize.cpp
@@ +3675,5 @@
>          return inlineSimdCheck(callInfo, native, type);
>        case SimdOperation::Fn_splat:
>          return inlineSimdSplat(callInfo, native, type);
>        case SimdOperation::Fn_extractLane:
> +        return inlineSimdExtractLane(callInfo, type);

Keep for consistency

@@ +3741,5 @@
>          return inlineSimdShift(callInfo, native, MSimdShift::rshForSign(GetSimdSign(type)), type);
>  
>          // Boolean unary.
>        case SimdOperation::Fn_allTrue:
> +        return inlineSimdAnyAllTrue(callInfo, /* IsAllTrue= */true, type);

Keep these for consistency.

::: js/src/jit/shared/BaselineCompiler-shared.cpp
@@ +19,5 @@
>    : cx(cx),
>      script(script),
>      pc(script->code()),
> +    ionCompileable_(jit::IsIonEnabled(cx) && CanIonCompileScript(cx, script)),
> +    ionOSRCompileable_(jit::IsIonEnabled(cx) && CanIonCompileScript(cx, script)),

Follow-up: Remove ionOSRCompileable_
Attachment #8951610 - Flags: review?(tcampbell) → review+
Hey Benjamin, I've hit a small problem with the WASM patches. This assertion [1] fails on try [2], but if undo the change for the ParseBranchTable function, the try build succeeds again [3]. I guess what happens here is that avoiding calling the copy-constructor for |WasmToken| changes the stack size for ParseBranchTable resp. its caller ParseExprBody, which then leads to a different behaviour for the recursion check in ParseExprBody. Or something like that...?
This is only a problem for Linux32-opt builds (no error on Linux32-debug or Linux64 [4]). So should I update the check in [1] to also allow OOM failures for Linux32-opt, or should I rather remove the complete build configuration check?

Thanks,
André

[1] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/js/src/jit-test/tests/wasm/compiler-frame-depth.js#25
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=45e8b4630080c8af0fa77a8c2edaa1fcf4af244b
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=39b2952bd9b11b67fd7b4a592f8371a9055ba405
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3de8d443c6af6cdd3aefd6cbd86a911e18c045b
Flags: needinfo?(bbouvier)
(In reply to Ted Campbell [:tcampbell] from comment #11)
> ::: js/src/jit/BaselineJIT.cpp
> @@ +949,5 @@
> >  }
> >  
> >  #ifdef JS_TRACE_LOGGING
> >  void
> > +BaselineScript::initTraceLogger(JSScript* script, const Vector<CodeOffset>& offsets)
> 
> This still looks used:
> https://searchfox.org/mozilla-central/rev/
> 74b7ffee403c7ffd05b8b476c411cbf11d134eb9/js/src/jit/BaselineJIT.cpp#966
> 

Hmm, the patch only removes the |JSRuntime*| argument for initTraceLogger. Did you mean to point to a different function?
Strange. Can you add debug annotations in wasmFullPass jit-tests/tests/lib/wasm.js to know which step in particular is failing? Of course I can't repro locally, so I'd suggest to just remove the builds option assertion if there's nothing weird that shows up in the try build logs.
Flags: needinfo?(bbouvier)
(In reply to Benjamin Bouvier [:bbouvier] from comment #14)
> Strange. Can you add debug annotations in wasmFullPass
> jit-tests/tests/lib/wasm.js to know which step in particular is failing?

Per [1], the error is thrown when |wasmTextToBinary| is called here [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c078a7681b8f1946c7ab2fc9900fa8e90665bca
[2] https://searchfox.org/mozilla-central/rev/5536f71c3833018c4f4e2c73f37eae635aab63ff/js/src/jit-test/lib/wasm.js#112

> Of course I can't repro locally, so I'd suggest to just remove the builds
> option assertion if there's nothing weird that shows up in the try build
> logs.

Me neither. (I forgot to mention that in comment #12, sorry!)
(In reply to André Bargull [:anba] from comment #13)
> Hmm, the patch only removes the |JSRuntime*| argument for initTraceLogger.
> Did you mean to point to a different function?

You are correct, I got myself confused looking at patch file. All the initTraceLogger stuff looks good to me.
Attachment #8951608 - Flags: review?(jorendorff) → review+
Priority: -- → P3
Rebased part 1 to apply cleanly on inbound. Carrying r+.
Attachment #8951607 - Attachment is obsolete: true
Attachment #8954694 - Flags: review+
Rebased part 2 to apply cleanly on inbound. Carrying r+.
Attachment #8951608 - Attachment is obsolete: true
Attachment #8954695 - Flags: review+
Rebased part 3 to apply cleanly on inbound and to apply review comments. Carrying r+.
Attachment #8951610 - Attachment is obsolete: true
Attachment #8954696 - Flags: review+
Rebased part 4 to apply cleanly on inbound. Carrying r+.
Attachment #8951611 - Attachment is obsolete: true
Attachment #8954697 - Flags: review+
Rebased part 5 to apply cleanly on inbound and to apply review comments and to adjust jit-test/tests/wasm/compiler-frame-depth.js per comment #14. Carrying r+.
Attachment #8951613 - Attachment is obsolete: true
Attachment #8954698 - Flags: review+
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab40910b4cc
Part 1: Remove unused parameters in GC code. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c92666cad03
Part 2: Remove unused parameters in built-in, frontend, and general files. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4b06e10ae6
Part 3: Remove unused parameters in js/src/jit. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ec168a898ac
Part 4: Remove unused parameters in js/src/vm. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4136715d683
Part 5: Remove unused parameters in js/src/wasm. r=bbouvier
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.