Performance regression back in 8.1.5+ (and still in 7.5.x) #2123
Description
Hey folks, following up on #2063, we ran into a similar regression and did some bisecting across 15+ versions that might help narrow things down.
For context, we hit ~7x slower Firestore emulator tests after a transitive upgrade to 7.5.x (via firebase-admin).
In 7.5.x, the issue seems to be that PR #2066 added _needsRecursiveFeatureResolution caching to _resolveFeaturesRecursive(), but resolveAll() still re-traverses the entire tree on every call. The 8.x branch has the fix, but PR #2068 that tried to backport it got reverted due to #2072.
Would it be possible to reconsider a backport to 7.5.x? This regression is pretty tricky to track down when it comes from a transitive dependency. Most people won't know to pin protobufjs specifically.
Also, while testing the 8.x experimental versions, we noticed that while 8.1.2-experimental seemed to perfom as well as 7.4.0, but 8.1.6 started to show some performance regression. Not as bad, but still observable.
In case it helps and for a rough idea of the performance differences, we tested a bunch of the 8.x versions against one of our test cases:
| Version | Duration | Notes |
|---|---|---|
| 7.4.0 | 2.5s | baseline |
| 7.5.x | ~55s | slow |
| 8.0.0 | ~23s | slow |
| 8.0.1-experimental | 2.3s | fast |
| 8.0.3-experimental | 2.9s | fast |
| 8.0.4-experimental | 3.0s | fast |
| 8.1.0-experimental | 4.3s | acceptable |
| 8.1.1-experimental | 5.0s | borderline |
| 8.1.3-experimental | >5s | timeout |
| 8.1.4-experimental | ~4-6s | flaky |
| 8.1.5-experimental | n/a | crash (TypeError: .google.protobuf.FieldOptions.targets: array expected) |
| 8.1.6-experimental | ~14s | slow |
Looks like 8.0.1 through 8.0.4-experimental have the fix and work well, but something regressed again in the 8.1.x line.
In the meantime, pinning to 7.4.0 works as a workaround:
{ "resolutions": { "protobufjs": "7.4.0" } }