Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More object field resolution / reduction optimizations #2029

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

kyri-petrou
Copy link
Collaborator

There are 2 main optimizations in this PR:

  1. Avoid usages of FunctionX and the number of branches within reduceObjectStep
  2. Use a mutable.HashMap in ObjectFieldResolver

Now, I'm somewhat conflicted about (2). For some reason, Scala's immutable HashMapBuilder is much slower than using a mutable Map. I realised this inefficiency after looking at some profiling graphs, where a lot of time was spend on the += method of the HashMapBuilder. It seems that even if the HashMapBuilder is mutable, the implementation copies the underlying arrays on each new entry (no idea why).

Since this is in our very hot path and we probably can't go changing the Scala std library, one way to optimize for it is to use a mutable HashMap, and change the Map type on ObjectStep from Map (i.e., immutable.Map) to collection.Map which is the base type for both mutable and immutable collections. This change is binary incompatible but largely source-compatible so most users should not need to do any changes to their code.

As I said though, I'm fairly conflicted about this change. It does offer a fairly sizeable performance improvement over the current implementation, but it could potentially leak mutability if misused; even if collection.Map doesn't expose any mutable APIs a user could potentially pattern match and get the mutable instance. Having said that, I don't know how much of an issue this actually is

series/2.x:

[info] Benchmark                                              Mode  Cnt    Score    Error  Units
[info] NestedZQueryBenchmark.multifieldSequentialQuery1000   thrpt    5  686.416 ± 30.976  ops/s
[info] NestedZQueryBenchmark.multifieldSequentialQuery10000  thrpt    5   61.166 ±  1.555  ops/s

PR (~10% improvement):

[info] Benchmark                                              Mode  Cnt    Score    Error  Units
[info] NestedZQueryBenchmark.multifieldSequentialQuery1000   thrpt    5  746.585 ± 21.516  ops/s
[info] NestedZQueryBenchmark.multifieldSequentialQuery10000  thrpt    5   65.220 ±  1.478  ops/s

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be against exposing a mutable map in the public API but the tricks of using collection.Map seems safe 🤔

.fold(NullStep: ReducedStep[R])(reduceStep(_, f, args, Left(aliasedName) :: path))

val info = fieldInfo(f, path, directives)
val field = fields.get(name) match {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the simplifications, the code is more readable

@kyri-petrou kyri-petrou merged commit ccb9716 into series/2.x Dec 7, 2023
10 checks passed
@kyri-petrou kyri-petrou deleted the more-executor-optimizations branch December 7, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants