Skip to content

chore: Tweak table switch#342

Closed
He-Pin wants to merge 1 commit intodatabricks:masterfrom
He-Pin:tweakPerf
Closed

chore: Tweak table switch#342
He-Pin wants to merge 1 commit intodatabricks:masterfrom
He-Pin:tweakPerf

Conversation

@He-Pin
Copy link
Copy Markdown
Contributor

@He-Pin He-Pin commented Apr 29, 2025

Try to tweak the perf regression in #313

@stephenamar-db would you like to test this one? Sorry for the regression

@stephenamar-db
Copy link
Copy Markdown
Collaborator

This seems to help, but we are still not all the way back.

# Run progress: 0.00% complete, ETA 00:02:00
# Fork: 1 of 1
Java HotSpot(TM) 64-Bit Server VM warning: -XX:ThreadPriorityPolicy=1 may require system level permission, e.g., being the root user. If the necessary permission is not possessed, changes to priority will be silently ignored.
# Warmup Iteration   1: 337.600 ms/op
# Warmup Iteration   2: 231.489 ms/op
Iteration   1: 230.718 ms/op
Iteration   2: 230.245 ms/op
Iteration   3: 230.376 ms/op
Iteration   4: 231.666 ms/op
Iteration   5: 230.690 ms/op
Iteration   6: 231.175 ms/op
Iteration   7: 231.921 ms/op
Iteration   8: 233.148 ms/op
Iteration   9: 233.244 ms/op
Iteration  10: 231.219 ms/op


Result "com.databricks.jsonnet.bundle.test.InternalJsonnetBuilderBenchmark.main":
  231.440 ±(99.9%) 1.611 ms/op [Average]
  (min, avg, max) = (230.245, 231.440, 233.244), stdev = 1.065
  CI (99.9%): [229.829, 233.051] (assumes normal distribution)


# Run complete. Total time: 00:02:02

REMEMBER: The numbers below are just data. To gain reusable insights, you need to follow up on
why the numbers are the way they are. Use profilers (see -prof, -lprof), design factorial
experiments, perform baseline and negative tests that provide experimental control, make sure
the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from the domain experts.
Do not assume the numbers tell you what you want them to tell.

Benchmark                             Mode  Cnt    Score   Error  Units
InternalJsonnetBuilderBenchmark.main  avgt   10  231.440 ± 1.611  ms/op

@stephenamar-db
Copy link
Copy Markdown
Collaborator

As an exercise, I removed the @switch from visitExpr0:

# Run progress: 0.00% complete, ETA 00:02:00
# Fork: 1 of 1
Java HotSpot(TM) 64-Bit Server VM warning: -XX:ThreadPriorityPolicy=1 may require system level permission, e.g., being the root user. If the necessary permission is not possessed, changes to priority will be silently ignored.
# Warmup Iteration   1: 334.742 ms/op
# Warmup Iteration   2: 227.862 ms/op
Iteration   1: 226.781 ms/op
Iteration   2: 226.250 ms/op
Iteration   3: 225.618 ms/op
Iteration   4: 225.475 ms/op
Iteration   5: 225.200 ms/op
Iteration   6: 225.317 ms/op
Iteration   7: 224.903 ms/op
Iteration   8: 224.888 ms/op
Iteration   9: 224.610 ms/op
Iteration  10: 225.989 ms/op


Result "com.databricks.jsonnet.bundle.test.InternalJsonnetBuilderBenchmark.main":
  225.503 ±(99.9%) 1.021 ms/op [Average]
  (min, avg, max) = (224.610, 225.503, 226.781), stdev = 0.675
  CI (99.9%): [224.482, 226.524] (assumes normal distribution)


# Run complete. Total time: 00:02:01

REMEMBER: The numbers below are just data. To gain reusable insights, you need to follow up on
why the numbers are the way they are. Use profilers (see -prof, -lprof), design factorial
experiments, perform baseline and negative tests that provide experimental control, make sure
the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from the domain experts.
Do not assume the numbers tell you what you want them to tell.

Benchmark                             Mode  Cnt    Score   Error  Units
InternalJsonnetBuilderBenchmark.main  avgt   10  225.503 ± 1.021  ms/op

@He-Pin
Copy link
Copy Markdown
Contributor Author

He-Pin commented Apr 29, 2025

There is an issue with the current visitExpr0, which means every time we retrieve the _tag we have to call with a invokeInterface, I think if we can tweak the class Inheritance structure, which will improve the performance.

  1. Make Expr an abstract class, then the _tag will be a field of its subclasses
  2. Make some classes, eg, Val.Null not a sub-class of Lazy.

At least I checked the jrsonnet , which does extend a common class Lazy for this, just plain value.

To tweak the expr , we can based it on some statistic numbers, eg , the top N most visited Expr nodes.

@He-Pin
Copy link
Copy Markdown
Contributor Author

He-Pin commented Apr 29, 2025

@stephenamar-db Which JVM version are you using? Thank you for looking into this. Does it relate to the fact that I add too many cases to the StaticOptimizer?

What if you revert some commits about StaticOptimizer?

@stephenamar-db
Copy link
Copy Markdown
Collaborator

stephenamar-db commented Apr 29, 2025

VM version: JDK 17.0.9, Java HotSpot(TM) 64-Bit Server VM, 17.0.9+11-LTS-jvmci-23.0-b21

@stephenamar-db
Copy link
Copy Markdown
Collaborator

let me try some things, sure

@He-Pin
Copy link
Copy Markdown
Contributor Author

He-Pin commented Apr 29, 2025

Scala 3 may be slower, are you test it with Scala 2.13?

@stephenamar-db
Copy link
Copy Markdown
Collaborator

all tests are with 2.13. Our code base is not compatible with 3 for now

@stephenamar-db
Copy link
Copy Markdown
Collaborator

I agree that the current hierarchy of Val class is not optimal, yeah.

@stephenamar-db
Copy link
Copy Markdown
Collaborator

What if I add a flag in Settings for your new Evaluator code, so you can continue to use it?

@He-Pin
Copy link
Copy Markdown
Contributor Author

He-Pin commented Apr 29, 2025

What if I add a flag in Settings for your new Evaluator code, so you can continue to use it?

I'm ok with it, thanks, our use cases is using sjsonnet to do data transformation through a pipeline, and the scripts is limited and long running.

@stephenamar-db
Copy link
Copy Markdown
Collaborator

stephenamar-db commented Apr 29, 2025

Also, yes the static optimizations in commit 60db9d0 made things worse too.

@stephenamar-db
Copy link
Copy Markdown
Collaborator

stephenamar-db commented Apr 29, 2025

My plan it to:

  • Rollback the static optimizations: 60db9d0
  • put the new evaluatorcode behind a setting

@He-Pin He-Pin deleted the tweakPerf branch April 29, 2025 04:51
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