1435569 - Reduce redundancy in CacheIR attachment spewing

Status: RESOLVED FIXED

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

Description

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Refactor CacheIR attachment spewing code to reduce redundancy

This patch mandates that all CacheIR IR generators create spew code to ensure
tracking by making the IRGenerator abstract virtual.

Comment 2

[Tom S [:evilpie]]

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.

Comment 3

[Jan de Mooij [:jandem]]

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).

Comment 4

[Ted Campbell [:tcampbell]]

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 5

[Tom S [:evilpie]]

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.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Change CacheIRSpewer to allow less redundency in spewing

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.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Note, unlike Ted's sketch, I didn't implement a getter for CacheIRSpewer::Guard.

Comment 8

[Ted Campbell [:tcampbell]]

Please wrap those nullptrs in something with a name. |constexpr char* CacheIRSpewer::NotAttached = nullptr;| or something.

Comment 9

[Tom S [:evilpie]]

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.

Comment 10

[Tom S [:evilpie]]

Actually Ted's suggestion might not work, because CacheIRSpewer is #ifdef on JS_CACHEIR_SPEW.

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Change CacheIRSpewer to allow less redundency in spewing

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.

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Give the "NotAttached" case a name

Comment 13

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8950921 [details] [diff] [review]
Change CacheIRSpewer to allow less redundency in spewing

Nit addressed, carrying evilpie's r+

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

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.

Comment 15

[Matthew Gaudet (he/him) [:mgaudet]]

Try build looks green -- setting checkin-needed flag

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efc83f62363e36f69e111b13300d185b5f6df1c9

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Andreea Pavel [:apavel]]

Backed out for build bustages at /builds/worker/workspace/build/src/js/src/jit/CacheIR.cpp:1906

push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9659c9a29139b0b66e1cfdeb26dd7735f4846006

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162444426&repo=mozilla-inbound&lineNumber=10565

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc2e391c8208908e44f76783416ea10491e8323

Comment 18

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Change CacheIRSpewer to allow less redundency in spewing

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.

Comment 19

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Give the "NotAttached" case a name

Comment 20

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Change CacheIRSpewer to allow less redundency in spewing

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.

Comment 21

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Give the "NotAttached" case a name

Comment 22

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 8951315 [details] [diff] [review]
Give the "NotAttached" case a name

Carrying evilpie's r+

Comment 23

[Matthew Gaudet (he/him) [:mgaudet]]

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)

Comment 24

[Ted Campbell [: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.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/114d2a3202c0
https://hg.mozilla.org/mozilla-central/rev/287ae6668ed2