1255008 - Flow sensitive Alias Analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

Our current alias analysis discards any flow information. As a result we get bad dependency information around loops and other control flow. In order to find more places that we can optimize having a flow sensitive Alias Analysis might make it possible to much more optimizations:
- Remove more loads after store
- Enable more hoisting of loads
- Remove loads depending on multiple stores
- Remove store before store (with non-fallible functions in between or eventually recover instructions)
- Annotate MCall with alias info (not depending on flow-sensitive AA, but allows better info)
- ...

There were definitely some trade-offs deciding on how to implement it. I suggest reading the comment above "FlowAliasAnalysis::analyze" first, before reading the thoughts/trade-offs list I made. I based myself on the code we currently have and the optimization that we want to do. Also I looked at the V8 source to know what they have and I also looked at Nbp his implementation of precise alias analysis. I don't think this implementation is the best AA that exists, but it definitely is a major improvement over what we have now, without impacting other passes much and with similar execution times than the current AA.

Some of the trade-offs were:
1) Keep a list of store dependencies VS keep and insert Phi nodes.

In my first iteration I started with inserting "memory phi" nodes, which combined the store dependencies and create a store chain. 
When finding a load that fully-aliased all stores of the phi, we could easily remove the load and create a real phi depending on the values of the store. That was the reason. That seemed to cause quite a mess in my first prototypes and also meant we had to keep track of a lot of phis, since we had to do that for every alias set category. Also it meant we had to support this at backedges... Also we had to keep this up to date through GVN and other optimization. I decided it was not worth the pain and decided to use a list of store dependencies. This list had the nice property that it was easy to detect when encountering a value we already had in the list (which we had to iterate again in the MPhi version). Decreasing time and complexity, while still getting the same dependencies. We keep the chain of store dependencies on the store itself. It points to the previous store dependencies.

2) Keep the notion of alias set category

The alias set category is a nice way to make the chain of stores that needs to get iterated smaller. I kept this in our new alias analysis. We need a little bit more place due to it (for every block we keep a list for every alias set category), but not that dramatic. We can still improve it, since we don't need the info of blocks where all predecessors have been visited, but I don't think it is worth the effort currently. I did adjust our storage requirements on the store itself. We only keep 1 alias set and a StoreAll alias set. This is because currently in our code almost all of our stores/loads have 1 specific alias set. As a result I could decrease the memory needed to store the store dependencies.

3) Keep the "one dependency" per load

This is also a way to decrease complexity in code and not to increase memory too much. In nbp his implementation this has been removed and contains uses and dependencies. Especially for a first improvement I didn't want this version to affect too much of the other passes. As a result it was much easier to keep this interface towards the other passes. Not doing that would involve serious changes to GVN and LICM and maybe more. In a way to control the possible fallout I kept the same interface. One dependency instruction storing a store instruction or storing a control instruction (for loop variant loads that depend on stores after the load). I did play with having a dependency on a store after the load, but that was error-prone and didn't work out.
The nice side-effect of this, is that we still can run both alias analysis passes! And my intention is to land it preffed off and let the fuzzers take a look!

4) Have mightAlias return NoAlias, MayAlias and MustAlias

Like normal AA, I wanted to have this. Gvn should actually call mightAlias and only replace the value if mightAlias is "MustAlias". Secondly for store after store removal and load of multiple stores (which will happen in AA itself), we need to have the MustAlias value, instead of having this logic in foldsTo.

Looking at V8, I definitely noticed that they also don't have a full precise AA. They remove their information on loops and looks quite good at intra block AA. They have some nice things we have don't have yet, like store after store removal. It was quite readable and understandable. I'm confident this AA can do all things v8 can and even more, without complicating it towards non-understandable and/or fixating on a full precise AA that handles all corner-cases perfect, but might be slow. Maybe I was even considering to add an upperbound on the "isLoopVariant" recursion.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug-alias-analysis

I was doubting between r? and f?. Went for r? since this is still preffed off. So we can be a little bit more eager in landing this. If possible I would also prefer address bigger items in follow-up bugs and let the "enable flow alias analysis" depend on it. But that is off course your call ;)

Comment 2

[Jan de Mooij [:jandem]]

This looks interesting but before I take a closer look at comment 0.. Do you see any benchmark improvements? How does it affect compilation time? :)

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Difference octane-richards in load dependencies

I looked at octane. We do have better dependency information, but it doesn't translate in performance improvements on octane. Seems our old one, just happened to catch everything already.

The log attached is for Octane-richards. It is visible we improved some to be loop invariant to MStart and some dependencies on similar stores, but lower id.
In most cases this doesn't help octane-richards since we haven't implemented foldsto on "loadunboxedobjectornull427" on "storeunboxedobjectornull232". Good follow-up bug and in that case having this better data might help. I'll open one!

Also I want to do removal of loads on multiple stores for simpler cases.
That should definitely help, but needs flow sensitive AA. I'm doing everything I listed in the opening bug as follow-up bugs, which needs flow sensitive AA. Therefore I wanted that finished/polish before going further. If you prefer to have those patches already, before reviewing that is fine. But they depend on flow sensitive AA. So if that is deemed incorrect the other patches won't be good either.

Also note that this new FlowAliasAnalysis is disabled by default. This patch doesn't enable it yet!

Comment 4

[Hannes Verschore [:h4writer]]

Timings from Tracelogger for AA compared to FlowAA. (AA pass only timings)

             After         Before
richards:    176.1664       73.7308
deltablue:   878.4188      312.74
zlib:       1377.49        174.9502
eb:         1735.1182      559.2468

So something between 2x to 10x slowdown. I assume the zlib case is because of multiple loops in each other. isLoopInvariant should probably be heuristically only be allowed X-levels deep...

Comment 5

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> zlib:       1377.49        174.9502

Very unexpected. Looking into. Seems creating the block store dependency information takes too much time. Might want to look into reusing info for blocks where all successors has been used. And decrease the default no of items in vectors.

Comment 6

[Hannes Verschore [:h4writer]]

Most time (60%) is in creating the block store information. This hurt esp. big graphs, since we create a BlockStore for every block. Some low hanging fruit shows already some improvement. Might be good to already start implementing reusing blocks when the successors have been used already.

           WIP   Previous  Before
richards:  147        176      73
deltablue: 619        878     312
zlib:      949       1377     174
eb:       1352       1735     559

Currently between 2x and 5x slower.

Comment 7

[Hannes Verschore [:h4writer]]

I added a little heuristic to decrease time spend in "merging of blocks". This helps a lot on zlib and eb. The heuristic is to not merge the store information of blocks that have more than 5 predecessors.
This gives me:

           WIP (reserve + prune pred)      WIP (reserve)   Previous  Before
richards:                         148                 147        176      73
deltablue:                        619                 619        878     312
zlib:                             696                 949       1377     174
eb:                              1190                1352       1735     559

That means between 2x and 4x slower

I think I'll prototype the improved store now. We still spend a lot of time in allocating.
Fixing that would bring us close to original speed. Only zlib is 3x slower, which might needs some extra heuristics.
richards: 45%
deltablue 44%
zlib: 33%
eb: 36%

The other part is merging blocks and iterating the load and store instructions.
In order to improve that we might to include some more heuristics. But first without heuristics ;)

Comment 8

[Hannes Verschore [:h4writer]]

Current progress (time to run the pass):

                      New      Old
richards:             144   |  45 (3.2x)
deltablue:            562   | 231 (2.4x)
zlib:                 570   | 170 (3.3x)
eb:                  1306   | 407 (3.2x)

I improved my measurements. (Some parts of AA weren't accounted for and something was counted that wasn't part of AA).

This is also a bit more encouraging. Always the same slowdown. The size of the graph influences the complexity similar to the old AA. Which was a bit concerning with the previous numbers.

The new datastructure is also implemented. Decreased the needed memory a lot. I still think there is some more room to decrease the time. I'm looking to be around 2x slower. Which would make this acceptable I think.

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8728429 [details] [diff] [review]
bug-alias-analysis

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

Seems like feedback would have been better. Removing review flag.
The changes are getting quite big, so I think it might be better to review the new version.
Making progress...

Comment 10

[Hannes Verschore [:h4writer]]

Current progress (time to run the pass):

                      New      Old
richards:             113   |  45 (2.5x)
deltablue:            453   | 231 (1.9x)
zlib:                 432   | 170 (2.5x)
eb:                  1010   | 407 (2.4x)

I broke the 3x barrier! So now we are in the 1x to 2.5x slower.

- Improved merging of information of blocks (still a time consuming task). Is using the "inWorklist" flag slow?
- Removed recursive call of isLoopInvariant and made it stop as soon as encountering 1 loopvariant instructions, instead of computing everything iterating over all.

Comment 11

[Hannes Verschore [:h4writer]]

              New     Old
richards:      85   |  45 (1.8x)
deltablue:    390   | 231 (1.6x)
zlib:         365   | 170 (2.1x)
eb:           794   | 407 (1.9x)

- Only keep track of all stores, instead of per category. This was hurting merging of blocks a lot.

This seem in the ballpark of what would be acceptable. Since AA doesn't take time at all (1%), this wouldn't make a big difference on compilation. And will probably become a little bit faster during polishing, since this was done quick and dirty and we allocate too much etc. I have to take my focus of this for Monday and Tuesday. Some pressing things need to get done. But I expect to polish it towards end of the week.

Comment 12

[Hannes Verschore [:h4writer]]

Attachment: Flow alias analysis

Next try.

1) Improves dependency info on octane. Doesn't cause gvn to remove more loads
2) This is a basic system to allow removal of stores after stores, loads depending on multiple stores. Real gains will happen in depending bugs.
3) Is not enabled by default yet. To give fuzzers a first chance
4) Compile times of FlowAA are better now (Keeping track of alias categories was wastefull). Still a little bit slower, but to put in perspective, we do more work now and have better info and this is only 1% of full ion compilation cost:

           FlowAA      AA
richards:   69     |   45   (1.5x)
deltablue: 305     |  231   (1.3x)
zlib:      288     |  170   (1.7x)
eb:        643     |  407   (1.6x)

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8733877 [details] [diff] [review]
Flow alias analysis

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

::: js/src/jit/JitOptions.cpp
@@ +86,5 @@
>      // Toggles whether Edge Case Analysis is gobally disabled.
>      SET_DEFAULT(disableEdgeCaseAnalysis, false);
>  
> +    // Toggles whether to use flow sensitive Alias Analysis.
> +    SET_DEFAULT(disableFlowAA, false);

SET_DEFAULT(disableFlowAA, true);

To make sure my comment is correct and flow sensitive AA is still disabled by default.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8733877 [details] [diff] [review]
Flow alias analysis

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

Ok, still some kinks to iron out.

Comment 15

[Hannes Verschore [:h4writer]]

Aha:

$ js --ion-aa=flow-sensitive --no-threads run-deltablue.js 
DeltaBlue: 55744
$ js --ion-aa=flow-sensitive --no-threads run-deltablue.js 
DeltaBlue: 56081
$ js --ion-aa=flow-sensitive --no-threads run-deltablue.js 
DeltaBlue: 55876


$ js --ion-aa=flow-insensitive --no-threads run-deltablue.js 
DeltaBlue: 55526
$ js --ion-aa=flow-insensitive --no-threads run-deltablue.js 
DeltaBlue: 55288
$ js --ion-aa=flow-insensitive --no-threads run-deltablue.js 
DeltaBlue: 55328

Comment 16

[Hannes Verschore [:h4writer]]

Attachment: Flow alias analysis

Comment 17

[Hannes Verschore [:h4writer]]

Attachment: Flow alias analysis

Removed the unrelated TL fixes

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8734338 [details] [diff] [review]
Flow alias analysis

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

Sorry for the delay! These large patches are really hard to review, but that's not a great excuse ;)

LGTM for the most part. Below the initial review comments (mostly nits). I didn't review every part closely yet, but I think this should cover most of it.

::: js/src/jit/AliasAnalysis.cpp
@@ +313,5 @@
>      return true;
>  }
> +
> +void
> +AliasAnalysis::spewDependencyList()

Can we merge this function with FlowAliasAnalysis::spewDependencyList?

::: js/src/jit/FlowAliasAnalysis.cpp
@@ +67,5 @@
> +class GraphStoreInfo
> +{
> +    // The current BlockStoreInfo while iterating the block untill,
> +    // it contains the store info at the end of the block.
> +    BlockStoreInfo *current_;

Nit: * to the left

@@ +77,5 @@
> +    // All BlockStoreInfo's that aren't needed anymore and can be reused.
> +    GraphStoreVector empty_;
> +
> +  public:
> +    GraphStoreInfo(TempAllocator& alloc)

Nit: |explicit|

@@ +86,5 @@
> +    bool reserve(size_t num) {
> +        if (!stores_.reserve(num))
> +            return false;
> +        for (size_t i = 0; i < num; i++)
> +            stores_.infallibleAppend(nullptr);

Nit: can use |if (!stores_.appendN(nullptr, num))|

@@ +136,5 @@
> +            bool release = true;
> +            for (size_t j = 0; j < block->getPredecessor(i)->numSuccessors(); j++) {
> +                if (block->getPredecessor(i)->getSuccessor(j)->id() > block->id()) {
> +                    release = false;
> +                    break;

I think we can |continue;| here and remove the |release| bool.

@@ +163,5 @@
> +
> +namespace {
> +
> +// Iterates over the flags in an AliasSet.
> +class AliasSetIterator

Please move this (and other similar code) to AliasAnalysis.h or a new AliasAnalysisShared.h header to avoid duplication.

@@ +226,5 @@
> +
> +static void
> +SetNotInWorkList(MDefinitionVector& worklist)
> +{
> +    for (size_t item = 0; item < worklist.length(); item++) {

Nit: no {}

@@ +452,5 @@
> +// get hoisted out (if there is no store between start loopheader and instruction).
> +//
> +// We visit the graph in RPO and keep track of the last stores in that block.
> +// Upon entering a block we merge the stores information of the predecessors.
> +// Only loopheaders are different, since we eagerly make it depends on the control instruction

Nit: s/depends/depend/, and wrap to 80 columns while you're here.

@@ +501,5 @@
> +        for (MPhiIterator def(block->phisBegin()), end(block->phisEnd()); def != end; ++def)
> +            def->setId(newId++);
> +
> +        BlockStoreInfo& blockInfo = stores_->current();
> +        {

Nit: maybe remove this {} ?

@@ +523,5 @@
> +                    if (!processLoad(blockInfo, *def))
> +                        return false;
> +                } else {
> +                    if (!processInstr(blockInfo, *def))
> +                        return false;

Nit: as processInstr just returns true, I'd prefer removing it for now until we really need it.

@@ +661,5 @@
> +// Given a load instruction and an initial store dependency list,
> +// find the most accurate store dependency list.
> +bool
> +FlowAliasAnalysis::improveDependency(MDefinition* ins, MDefinitionVector& input,
> +                                     MDefinitionVector& output)

Nit: inputStores, outputStores?

@@ +806,5 @@
> +    return true;
> +}
> +
> +bool
> +FlowAliasAnalysis::deferImproveDependency(MDefinitionVector& output)

Nit: argument is unused

@@ +811,5 @@
> +{
> +    // Look if the store depends only on 1 non finished loop.
> +    // In that case we will defer until that loop has finished.
> +    return loop_ && output_.length() == 1 &&
> +           output_[0]->isControlInstruction() &&

Nit: maybe rename output_ to stores_? It wasn't immediately clear what kind of definitions were in |output|.

@@ +826,5 @@
> +    // That means the store with the maximum id.
> +    MDefinition* max = output_[0];
> +    MDefinition* maxNonControl = nullptr;
> +    for (size_t i = 0; i < output_.length(); i++) {
> +        MDefinition *ins = output_[i];

Nit: * to the left

@@ +829,5 @@
> +    for (size_t i = 0; i < output_.length(); i++) {
> +        MDefinition *ins = output_[i];
> +        if (max->id() < ins->id())
> +            max = ins;
> +        if (!ins->isTest()) {

s/isTest/isControlInstruction/ ? Or assert !isControlInstruction if !isTest.

@@ +840,5 @@
> +    // the control instruction of the loop header is returned.
> +    // That id is higher than any store inside the loopheader itself.
> +    // Fix for dependency on item in loopheader, but before the "test".
> +    // Which would assume it depends on the loop itself.
> +    if (maxNonControl != max && maxNonControl && max) {

Nit: I think we can remove the null check for |max|, or else the previous loop would crash.

@@ +884,5 @@
> +
> +// Determines if a load is loop invariant.
> +//
> +// Get the last store dependencies of the backedge of the loop
> +// and follow the store chain untill finding the aliased stores.

Nit: until

@@ +904,5 @@
> +    // To make sure the improve dependency stops at this loop,
> +    // set the loop control instruction as dependency.
> +    MDefinition* olddep = load->dependency();
> +    load->setDependency(store);
> +    {

Nit: no {}

@@ +958,5 @@
> +    }
> +
> +    // Optimization for consecutive blocks.
> +    if (block->numPredecessors() == 1) {
> +        if (block->getPredecessor(0)->numSuccessors() == 1) {

Nit: maybe store block->getPredecessor(0) in a local variable, as it's used at least 4 times.

@@ +961,5 @@
> +    if (block->numPredecessors() == 1) {
> +        if (block->getPredecessor(0)->numSuccessors() == 1) {
> +            stores_->swap(block, block->getPredecessor(0));
> +            return true;
> +        } else if (block->getPredecessor(0)->numSuccessors() > 1) {

Nit: no else after return

@@ +962,5 @@
> +        if (block->getPredecessor(0)->numSuccessors() == 1) {
> +            stores_->swap(block, block->getPredecessor(0));
> +            return true;
> +        } else if (block->getPredecessor(0)->numSuccessors() > 1) {
> +            BlockStoreInfo &predInfo = stores_->get(block->getPredecessor(0));

Nit: move & to the left, also below

@@ +973,5 @@
> +    // Heuristic: in most cases having more than 5 predecessors,
> +    // increases the number of dependencies too much to still be able
> +    // to do an optimization. Therefore don't do the merge work.
> +    if (block->numPredecessors() > 5) {
> +        if (!blockInfo->append(block->getPredecessor(0)->lastIns()))

Why is it (always) okay to use the first predecessor's last instruction? Could use a comment.

::: js/src/jit/MIR.h
@@ +371,5 @@
>      {
>      }
>  
>    public:
> +    static const char * Name(size_t i) {

Nit: s/char */char*/. Also let's move this to the cpp file, MIR.h is huge.

@@ +426,5 @@
> +typedef Vector<MInstruction*, 6, JitAllocPolicy> MInstructionVector;
> +
> +class StoreDependency : public TempObject
> +{
> +    MDefinitionVector all_;

How many instructions do we expect here? A vector with space for 6 elements is fairly large, if we have one for every store.

@@ +429,5 @@
> +{
> +    MDefinitionVector all_;
> +
> +  public:
> +    StoreDependency(TempAllocator& alloc)

Nit: |explicit|

@@ +434,5 @@
> +      : all_(alloc)
> +    { }
> +
> +    bool init(MDefinitionVector& all) {
> +        if (!all_.appendAll(all))

Can we use a move constructor here, to avoid an extra copy in some cases? Something like:

StoreDependency(MDefinitionVector&& all)
  : all_(Move(all))
{ }

I think that works because the caller clear()s the original Vector? The move constructor will also clear |all| so the explicit clear() is no longer necessary (we could assert the vector is empty there).

@@ +466,5 @@
>      Range* range_;                 // Any computed range for this def.
>      MIRType resultType_;           // Representation of result type.
>      TemporaryTypeSet* resultTypeSet_; // Optional refinement of the result type.
>      union {
> +        uintptr_t dependency_;     // Implicit dependency (store, call, etc.) of this instruction.

Instead of the pointer tagging, can we have 2 union values?

MDefinition* loadDependency_;
StoreDependency* storeDependency_;

@@ +918,5 @@
> +    }
> +    void setDependency(MDefinition* dependency) {
> +        dependency_ = uintptr_t(dependency);
> +    }
> +    void setStoreDependency(StoreDependency *dependency) {

Nit: move the * to the left

@@ +921,5 @@
> +    }
> +    void setStoreDependency(StoreDependency *dependency) {
> +        dependency_ = uintptr_t(dependency) | 1;
> +    }
> +    StoreDependency* getStoreDependency() {

Nit: s/getStoreDependency/storeDependency/ is more consistent with dependency()

@@ +943,5 @@
>          return isEffectful();
>      }
>  #endif
> +
> +    enum AliasType {

nit: |enum class AliasType|, for extra type safety and less namespace pollution. It's a bit more verbose, but I think the extra compile time checks are worth it.

Comment 19

[Hannes Verschore [:h4writer]]

Attachment: Flow alias analysis

Addresses review comments:
- Created AliasAnalysisShared and move things in there
- Made AliasType an enum class, uncovered some faults!
- Made an union out of dependency_
- StoreDependency is now a Vector with length 1
- Renamed all arguments of improveXXX to be load, inputStores, outputStores ...
- ...

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8739466 [details] [diff] [review]
Flow alias analysis

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

Sorry for the delay.

::: js/src/jit/AliasAnalysis.h
@@ +6,5 @@
>  
>  #ifndef jit_AliasAnalysis_h
>  #define jit_AliasAnalysis_h
>  
> +#include "jit/AliasAnalysisShared.h"

The AliasAnalysisShared.h/cpp files are not in the patch. Could you post an updated patch with these files? They're probably trivial so I can review quickly.

::: js/src/jit/FlowAliasAnalysis.cpp
@@ +919,5 @@
> +
> +    // Heuristic: in most cases having more than 5 predecessors,
> +    // increases the number of dependencies too much to still be able
> +    // to do an optimization. Therefore don't do the merge work.
> +    // For easyness we take an non-dominant always existing instructions.

Nit: s/easyness/simplicity and s/instructions/instruction/

::: js/src/jit/MIR.h
@@ -344,5 @@
>          Element           = 1 << 1, // A Value member of obj->elements or
>                                      // a typed object.
>          UnboxedElement    = 1 << 2, // An unboxed scalar or reference member of
> -                                    // a typed array, typed object, or unboxed
> -                                    // object.

Nit: don't remove these 2 lines.

@@ +903,5 @@
> +            return nullptr;
> +        return loadDependency_;
> +    }
> +    void setDependency(MDefinition* dependency) {
> +        MOZ_ASSERT (!getAliasSet().isStore());

Nit: remove space before (, also a few times below.

Comment 21

[Hannes Verschore [:h4writer]]

Attachment: AliasAnalaysisShared

Indeed I forgot to add them. Hereby in separate patch. I'll land them together.

Comment 22

[Jan de Mooij [:jandem]]

Comment on attachment 8744845 [details] [diff] [review]
AliasAnalaysisShared

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

Thanks

Comment 23

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0c1d923c29220f227da3e520fcae6c737c60566
Bug 1255008: IonMonkey - Add a by default disabled flow sensitive alias analysis pass, r=jandem

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c0c1d923c292

Comment 25

[Hannes Verschore [:h4writer]]

Attachment: Fix for kraken score

I landed and added a line on AWFY.

I didn't expected regressions wrt less precise dependency information. FlowAA need at least give the same or better information. This patch fixes the regression on kraken-oscillator and very possible the others too.

Comment 26

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e237f92c4a2f0ec545371af4759ddbdf6ff12df
Bug 1255008: IonMonkey - Don't alias when intersection of categories is empty and typo, r=jandem

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e237f92c4a2

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

most likely this caused an increase in num_constructors during the build:
https://treeherder.mozilla.org/perf.html#/alerts?id=1054

Comment 29

[Hannes Verschore [:h4writer]]

Attachment: Num_constructors testcase

(In reply to Joel Maher (:jmaher) from comment #28)
> most likely this caused an increase in num_constructors during the build:
> https://treeherder.mozilla.org/perf.html#/alerts?id=1054

Are you sure "num_constructors" is measuring what it says?
It took me a while to question that. I found no place where I added a static constructor and had to strip the whole patch to find the issue. Seems like just adding two .cpp files caused this. I've uploaded the patch that increase the "num_constructors" from 22 to 23 on a shell build on revision d2af90b2dba2.

Comment 30

[Worcester12345]

"perf" keyword?