Poor Performance of Classes vs Plain JavaScript Objects in Immer 10.0.2 vs 6.0.3

[bradedelman]

🐛 Bug Report

This is sample that illustrates a problem we are having with our real use of Immer, but is simplified for this issue report.

We have a piece of data with 12 simple numeric fields.
We have a version of it that is a plain JavaScript object.
We have a version of it that is a JavaScript class.

In Immer 6.0.2, mutating with produce is about the same speed with both the Class Instance and Plain JS Object.
In Immer 10.0.2, mutating with produce, the class instance is about 5x slower than the Plain JS Object.

Note that in Immer 9.0.21, plain objects are slower than 6.0.2, but about the same speed as Class Instances, so it seems this is an active area of development. It's nice that 10.0.2 is faster than 6.0.2 for Plain JS Object. However, the perf issue that remains for class instances is significant.

We are targeting 120,000 object mutations per second (on top of other work) and that 5x multiple has a meaningful, negative performance impact. (We have 2000 objects and there's a target 60 FPS update).

The class is very simple here. Nothing fancy. No getters, etc.

class Foo {
	[immer.immerable] = true;	
  	a = 1;
  	b = 2;
  	c = 3;
  	d = 4;
  	e = 5;
  	f = 6;
  	g = 7;
  	h = 8;
  	i = 9;
  	j = 10;
  	k = 11;
  	l = 12;
}

vs an instance of this plain JS Object:

{
  "a": 1,
  "b": 2,
  "c": 3,
  "d": 4,
  "e": 5,
  "f": 6,
  "g": 7,
  "h": 8,
  "i": 9,
  "j": 10,
  "k": 11,
  "l": 12
}

Link to repro

(be sure to view the JS Fiddle Console to see the console output)

Here's our sample code running JS Fiddle with Immer 6.0.3:
https://jsfiddle.net/csz37eb0/1/

"class", 232.2000002861023
"object", 221
"x slower with class", 1.0506787343262547

Here's our sample code running JS Fiddle with Immer 10.0.2:
https://jsfiddle.net/bop856g1/2/

"class", 489.5
"object", 93.7000002861023
"x slower with class", 5.2241195144649675

To Reproduce

Sample code provided in the JS Fiddles.

Observed behavior

mutating a simple class instance is 5x slower than equivalent plain JS Object.
5x is significant. Also, Immer 6.0.3 had them about the same speed.

Expected behavior

Same or almost same speed for a simple class like this.

Environment

• Immer version 10.0.2
• Mac M1
• Chrome Version 116.0.5845.187.

[mweststrate]

This likely relates to the setUseStrictShallowCopy(true); has been defaulted back to false in 10.0 for plain objects. Possibly it should be possible to enable loose mode for classes as well, but for classes it is a bit more dangerous than plain objects.

[mweststrate]

Closing for inactivity

[bradedelman]

inactivity? I mean, who is supposed to work on this? I reported the issue, and it's still an issue. I'd like to see this worked on :-)

[mweststrate]

There was no response to the follow-up question I asked.

And if you really want to play that card; no one in particular is is supposed to work on this, the software is provided as-is and I owe my time to no one. Sorry for being salty; and maybe you didn't mean it that way, but entitlement of users to a specific issue to be worked on after you have already given your work and time away for free is really frustrating as a maintainer. I mean, in the end you are probably on someones payroll to make whatever product you are working on faster, I'm not. So feel free to investigate the difference and put up a PR. "I'd like to see this worked on" is something you might be able to bring up as a convincing motivation at your job, but not in an open source community.

[bradedelman]

I'm not trying to be snarky or "play a card"! What was the question asked? The only comment I see

"This likely relates to the setUseStrictShallowCopy(true); has been defaulted back to false in 10.0 for plain objects. Possibly it should be possible to enable loose mode for classes as well, but for classes it is a bit more dangerous than plain objects."

doesn't read like a question.

I spent quite a lot of time tracking down and writing the examples that show the performance issue. If there's a way I can help, I'm open to that, though I was hoping that the developers who had done the work and had the context might be able to improve it, given it was faster in the past.

Thanks.

[mweststrate]

Ok that is good to know.

The question was basically whether setUseStrictShallowCopy was used or not. 10.0.4 might expose different perf characteristics btw, as the iteration mechanism slightly changed (didn't find a difference in my tests). Over time we did change the iteration method a few times, and the bottleneck is probably getOwnPropertyDescriptors right now, which is slow but also the one that doesn't miss any props, nor finds anything that it shouldn't (properties coming from a prototype etc). So I'd expect that a large deal of the speed loss is the cost of being correct in the corner cases (which is also why the option was added to disable parts of that)

[bradedelman]

You can see the code from the tests in the jsfiddle's linked in the original issue report.
setUseStrictShallowCopy(true);
was not used.

If I add that to the version of the test that used Immer 10.0.2, indeed it is faster. 1.21x slower with class vs. 5.33x slower without it. So indeed, that is the lion's share of the difference.

And yes, do think I saw getOwnPropertyDescriptors show up in my profiling.

Any advice on how to determine whether our use of Immer will be impacted by setUseStrictShallowCopy(true);?

You do explain it somewhat above, but what I'd be curious for is how to most easily determine whether our logic is dependent on the factors you mention.

[mweststrate]

This solution might be worth further investigation https://jsfiddle.net/ht9xm2zf/, see #1107. Didn't think through yet what it'll break though.

[bradedelman]

right, that's what I had just tried. and yes, it makes it faster.
however, to consider it, I'll have to "think through yet what it'll break".
since we ran ok with immer 6.0.2, does that suggest we are likely not going to break?

also, does the change impact non-classes?

[mweststrate]

Our suite of exotic scenarios grew vastly over the years, so things that might not have been working correctly in 6, but were irrelevant for your case, might now fail simply because they are tested, where they weren't in the past. E.g. own, string based property is like 98% of the cases that Immer needs to handle. But the remaining 2% is a state explosion of string / number / symbol * enumerable / non-enumerable * own / proto * value / getter / setter / getter + setter configurations that all have impact on reflection. So that is 48 scenarios that need to be work correctly, multiplied by the flags Immer itself offers. And not all of them were considered in the past.

[bradedelman]

ok, thanks. we are going to test with setUseStrictShallowCopy(true); and I suspect that will address it for us.
sorry that I did not understand that you were asking us to try that last September. I thought you were leaving an "internal comment".
I think given the information provided and our initial resaults/understanding, it's ok if you prefer to close the issue. I guess the alternative would be to provide a way to "test if this object requires the more complex logic" so that there was a clear(er) criteria for evaluating use of setUseStrictShallowCopy.

[mweststrate]

Did you try the fiddle I linked above (note: with useStrictShallowCopy(false))? It does restore the same perf characteristics as before, it might be worth validating that in your real project (version 10.0.4-beta). So that does link interesting to explore further and rollout. I'll need some time to properly create that in a backward compatible manner etc.

[bradedelman]

yes, I tried it and commented yesterday (above) that indeed it restored the performance.

[mweststrate]

Perf improvements have landed as https://github.com/immerjs/immer/releases/tag/v10.1.0! Thanks for surfacing this @bradedelman (despite my lacklustre response)!