Closed Bug 1435569 Opened 4 years ago Closed 4 years ago

Reduce redundancy in CacheIR attachment spewing

Categories

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

All
Unspecified
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mgaudet, Assigned: mgaudet)

Details

Attachments

(2 files, 6 obsolete files)

The implementations of trackAttached() and trackNotAttached() in the subclasses of IRGenerator are all very self similar, which suggests that there's probably commonality to be extracted.
Assignee: nobody → mgaudet
Severity: normal → enhancement
This patch mandates that all CacheIR IR generators create spew code to ensure
tracking by making the IRGenerator abstract virtual.
Attachment #8948261 - Flags: review?(evilpies)
Really nice simplification, but I am concerned that the compiler will keep around the vtable even when we don't build with spewing enabled. Can you check that? We also usually add the override specifier.
I wonder if we could do something where the derived classes call a method in the base class (IRGenerator) and pass a lambda to spew the contents? We've been trying to avoid virtual functions in the CacheIR code and we're trying to remove them from Ion too (bug 1425580).
Priority: -- → P3
My alternate unrefined proposal:

> void
> InstanceOfIRGenerator::trackAttached(const char* name)
> {
> #ifdef JS_CACHEIR_SPEW
>     if (CacheIRSpewer::Guard sp = CacheIRSpewer::getICSpewer(this, name)) {
>         sp.valueProperty("lhs", lhsVal_);
>         sp.valueProperty("rhs", ObjectValue(*rhsObj_));
>     }
> #endif
> }

I'd also like to revisit the question of can we merge trackAttached and trackNotAttached, instead of nullptr or some named sentinel.
Comment on attachment 8948261 [details] [diff] [review]
Refactor CacheIR attachment spewing code to reduce redundancy

Resetting the review request for now. Combining trackAttached and trackNotAttached sounds good.
Attachment #8948261 - Flags: review?(evilpies)
This patch adds an inner class to CacheIRSpewer that manages access to the
CacheIRSpewer and dramatically simplifies the consuming code.

Note: this changes the CacheIRSpewer to no longer use LockGuard; instead
the raw mutex is managed by CacheIRSpewer. This is because the RAII nature
of CacheIRSpewer::Guard prevents constructing the LockGuard if-and-only-if
the spewer is enabled.
Attachment #8950796 - Flags: review?(evilpies)
Attachment #8948261 - Attachment is obsolete: true
Note, unlike Ted's sketch, I didn't implement a getter for CacheIRSpewer::Guard.
Please wrap those nullptrs in something with a name. |constexpr char* CacheIRSpewer::NotAttached = nullptr;| or something.
Comment on attachment 8950796 [details] [diff] [review]
Change CacheIRSpewer to allow less redundency in spewing

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

This actually extremely nice looking, thanks for cleaning this up! Please include Ted's nullptr suggestion as well.

::: js/src/jit/CacheIRSpewer.h
@@ +46,5 @@
> +
> +    class MOZ_RAII Guard {
> +        CacheIRSpewer& sp_;
> +        const IRGenerator& gen_;
> +        const char * name_;

Super nit: * goes with the type.
Attachment #8950796 - Flags: review?(evilpies) → review+
Actually Ted's suggestion might not work, because CacheIRSpewer is #ifdef on JS_CACHEIR_SPEW.
This patch adds an inner class to CacheIRSpewer that manages access to the
CacheIRSpewer and dramatically simplifies the consuming code.

Note: this changes the CacheIRSpewer to no longer use LockGuard; instead
the raw mutex is managed by CacheIRSpewer. This is because the RAII nature
of CacheIRSpewer::Guard prevents constructing the LockGuard if-and-only-if
the spewer is enabled.
Attachment #8950796 - Attachment is obsolete: true
Comment on attachment 8950921 [details] [diff] [review]
Change CacheIRSpewer to allow less redundency in spewing

Nit addressed, carrying evilpie's r+
Attachment #8950921 - Flags: review+
Comment on attachment 8950922 [details] [diff] [review]
Give the "NotAttached" case a name

Because the NotAttached case is referred to externally, in contexts where CacheIRSpewer isn't already imported, this patch puts the name inside the IRGenerator.
Attachment #8950922 - Flags: review?(evilpies)
Attachment #8950922 - Flags: review?(evilpies) → review+
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cff9cca3774
Change CacheIRSpewer to allow less redundency in spewing r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa3a63240665
Give the "NotAttached" case a name r=evilpie
Keywords: checkin-needed
This patch adds an inner class to CacheIRSpewer that manages access to the
CacheIRSpewer and dramatically simplifies the consuming code.

Note: this changes the CacheIRSpewer to no longer use LockGuard; instead
the raw mutex is managed by CacheIRSpewer. This is because the RAII nature
of CacheIRSpewer::Guard prevents constructing the LockGuard if-and-only-if
the spewer is enabled.
Attachment #8950921 - Attachment is obsolete: true
Attachment #8950922 - Attachment is obsolete: true
Attachment #8951311 - Attachment is obsolete: true
Flags: needinfo?(mgaudet)
This patch adds an inner class to CacheIRSpewer that manages access to the
CacheIRSpewer and dramatically simplifies the consuming code.

Note: this changes the CacheIRSpewer to no longer use LockGuard; instead
the raw mutex is managed by CacheIRSpewer. This is because the RAII nature
of CacheIRSpewer::Guard prevents constructing the LockGuard if-and-only-if
the spewer is enabled.
Attachment #8951310 - Attachment is obsolete: true
Comment on attachment 8951315 [details] [diff] [review]
Give the "NotAttached" case a name

Carrying evilpie's r+
Attachment #8951315 - Flags: review+
Comment on attachment 8951314 [details] [diff] [review]
Change CacheIRSpewer to allow less redundency in spewing

This patch addresses a static analysis failure by changing the guard to capture a const reference. This means copy elision occurs, which means no temporary, and the static analysis passes (https://treeherder.mozilla.org/#/jobs?repo=try&revision=20bee090afa6cdb4f4ee0204e066a6db093b7c64&selectedJob=162453749)
Attachment #8951314 - Flags: review?(tcampbell)
Comment on attachment 8951314 [details] [diff] [review]
Change CacheIRSpewer to allow less redundency in spewing

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

Looks like a reasonable fix for the static analysis bug.
Attachment #8951314 - Flags: review?(tcampbell) → review+
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/114d2a3202c0
Change CacheIRSpewer to allow less redundency in spewing. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/287ae6668ed2
Give the "NotAttached" case a name r=evilpie
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/114d2a3202c0
https://hg.mozilla.org/mozilla-central/rev/287ae6668ed2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.