Speed up script evaluation by prehashing & interning identifier strings #4648

ochafik · 2023-05-23T19:25:53Z

This PR substantially speeds up variable bindings & lookups by interning variable names (allows comparing string pointers instead of string contents) and precomputing their hash, using a new Identifier class that's built during AST creation and replaces most std::string name uses.

Result: a model like poly_attached_inlined.scad (heavy scripting w/ BOSL2) gets 10% faster preview (measured w/ -o out.echo).

Benchmark command-line & results

hyperfine --warmup 1 --runs 1 -L build buildMaster,build \
    '{build}/OpenSCAD.app/Contents/MacOS/OpenSCAD poly_attached_inlined.scad -o out.echo'

Benchmark 1: buildMaster/OpenSCAD.app/Contents/MacOS/OpenSCAD poly_attached_inlined.scad -o out.echo
  Time (abs ≡):         9.463 s               [User: 9.395 s, System: 0.061 s]
 
Benchmark 2: build/OpenSCAD.app/Contents/MacOS/OpenSCAD poly_attached_inlined.scad -o out.echo
  Time (abs ≡):         8.502 s               [User: 8.440 s, System: 0.057 s]
 
Summary
  'build/OpenSCAD.app/Contents/MacOS/OpenSCAD poly_attached_inlined.scad -o out.echo' ran
    1.11 times faster than 'buildMaster/OpenSCAD.app/Contents/MacOS/OpenSCAD poly_attached_inlined.scad -o out.echo'

Notes:

Removed builtin function lookup cache from this PR (speedup w/ it was ~15%) ~~Some extra speedup is had by caching the resolution of builtin functions inside the Identifier class (caching other kinds of values is more complex, w/ unclear payoffs so far).~~
While interning of variable names strings will grow memory of the app over time, it's expected to be negligible.
As hinted by @jordanbrown0 below, we could technically pre-resolve variables by keeping an index into the context stack, then a variable slot index within the context. I've tried various versions of that (e.g. this one remembering how many context parent escalations are needed) but it's tricky (same variable not always resolved at the same level, e.g. first iteration of LcForC has a different context layout than subsequent ones) and the end benefit wasn't significant.

jordanbrown0 · 2023-05-23T20:35:28Z

[ Without reviewing the implementation. ]

Precalculating the hashes seems entirely plausible. From what you're saying, it sounds like you're thinking of this as caching, and from a quick glance it looks like you aren't touching the parser. You could do this precalculation at parse time.

In theory you could resolve lexical-scope (non-$) variable and function references to an offset into a stack, no hashing required. Or, because of context capture, more likely you'd want to resolve into an index into a stack of contexts and an index into that context.

$ variables are probably require hashing.

Is this worth the complexity? For most OpenSCAD programs, probably not, because the rule of thumb is that execution time is trivial. For programs using complex libraries like BOSL2, yeah, probably worthwhile.

ochafik · 2023-05-23T21:53:34Z

@jordanbrown0 Thanks for the rapid feedback! I've updated the poor wording, the code indeed precomputes the hashes when building the ASTNodes.

In theory you could resolve lexical-scope (non-$) variable and function references to an offset into a stack, no hashing required. Or, because of context capture, more likely you'd want to resolve into an index into a stack of contexts and an index into that context.

Ah of course, thanks! I was a bit scared of magical semantics but as you point out it's probably limited to $ vars.

Is this worth the complexity?

I'll take a look, not sure how much more complex it would need to be.

For most OpenSCAD programs, probably not, because the rule of thumb is that execution time is trivial. For programs using complex libraries like BOSL2, yeah, probably worthwhile.

Agree for most programs, but we do get repeated complains from people who want faster previews, and I tend to be completely constrained by evaluation time now that I've tasted the sweet BOSL2 fruit (and that I've done all I could on the rendering front, e.g. integrating manifold, fast-csg).

The model linked from this PR takes 37sec to preview with the PR's change vs. 48sec before... then only 6 extra seconds to render w/ Manifold 🤦‍♂️.

add missing include

kintel · 2023-06-30T02:58:32Z

src/core/Context.cc

 {
-  if (is_config_variable(name)) {
-    return session()->lookup_special_function(name, loc);
+  if (name.resolved_builtin_function) {


It does feel a little odd to add builtin function caching to the Identifier class. Would a separate function cache make sense, just for the sake of separation of concerns?

Btw, the rest of this PR looks good, modulo test failures, so the quickish way forward could be to split out the function caching into a separate PR to move this one forward.

@kintel sorry it took me so long to get back to this, fixed tests + removed the function caching bit as suggested

kintel · 2023-11-21T02:45:40Z

src/core/Context.cc

    return session()->lookup_special_function(name, loc);
  }
  for (const Context *context = this; context != nullptr; context = context->getParent().get()) {
    boost::optional<CallableFunction> result = context->lookup_local_function(name, loc);
    if (result) {
-      return result;
+      return std::move(result);


I wouldn't worry about this, but just a note to whomever is reading this that at some point we should probably go through our usage of std::move(). In this case, moving shouldn't be needed due to NRVO copy elision, although the effect is likely negligible.

kintel · 2023-11-21T03:06:22Z

src/core/Identifier.cc

+  auto it = interned_names_with_hash.find(name);
+  if (it == interned_names_with_hash.end()) {
+    it = interned_names_with_hash.emplace(name, std::make_pair(name, std::hash<std::string>{}(name))).first;
+  }


Nit: I think all our supported compilers have C++17 now, so you should be able to this, if appropriate:
interned_names_with_hash.try_emplace(name, std::make_pair(name, std::hash<std::string>{}(name)))

kintel · 2023-11-21T03:10:54Z

src/core/Identifier.h

+  bool is_config_variable_ = false;
+  std::string *interned_name_ptr = NULL;
+  size_t hash = 0;
+  Location loc;


Another nit: We don't really have a convention for private variables. Old code used no special syntax and prefers prefixing with this-> on access. Newer code seems to have adopted _ suffix as popularized by certain style guides. The _ variant makes code less ambiguous to read, so I'm very slightly tending towards it myself. When modifying existing classes, we should, IMO, try to adhere to one of the two though.

kintel · 2023-11-21T03:12:09Z

For some reason, one of the tests seem to crash on Windows. Needs looking into.

ochafik changed the title ~~Speed up script evaluation by caching string hashes and builtin function lookups~~ Speed up script evaluation by precomputing identifier hashes and caching builtin function lookups May 23, 2023

ochafik marked this pull request as draft May 23, 2023 21:58

ochafik mentioned this pull request May 25, 2023

Integrate non-CSG speedup initiatives [Do not merge] #4654

Draft

ochafik added 7 commits May 30, 2023 18:26

Identifier: pre-hashed string to speed up lookups!

c5ac2fd

add missing include

Unordered set in Let

7f58deb

Cache builtin function lookups in Identifier

8bad120

Add some missing Identifier refs

3afb127

Add Identifier::location()

f65faa5

Move is_config_variable from ContextFrame -> Identifier

9a900a1

Intern Identifier's name for faster == (speeds up hashtable lookups)

02704a7

ochafik force-pushed the identifiers branch from 1c59d13 to 02704a7 Compare May 30, 2023 17:26

Delete export_stl copy.cc

577092e

ochafik marked this pull request as ready for review May 30, 2023 18:31

ochafik changed the title ~~Speed up script evaluation by precomputing identifier hashes and caching builtin function lookups~~ Speed up script evaluation by prehashing & interning identifier string and caching builtin function lookups May 30, 2023

Skip empty context frames during var resolution

f8c1aef

kintel reviewed Jun 30, 2023

View reviewed changes

ochafik added 2 commits November 17, 2023 22:22

Remove Identifier's builtin function cache

e125743

Merge remote-tracking branch 'origin/master' into identifiers

14e2711

ochafik changed the title ~~Speed up script evaluation by prehashing & interning identifier string and caching builtin function lookups~~ Speed up script evaluation by prehashing & interning identifier strings Nov 17, 2023

ochafik added 3 commits November 17, 2023 23:32

Don't trust Context::empty() (fixes most tests)

53b649f

[identifiers] Simplify Identifier::update + cache hash

574c344

Remove calls to untrustworthy stack element empty() method

3a4156b

kintel reviewed Nov 21, 2023

View reviewed changes

kintel approved these changes Nov 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up script evaluation by prehashing & interning identifier strings #4648

Speed up script evaluation by prehashing & interning identifier strings #4648

ochafik commented May 23, 2023 •

edited

jordanbrown0 commented May 23, 2023

ochafik commented May 23, 2023

kintel Jun 30, 2023

kintel Jun 30, 2023

ochafik Nov 21, 2023

kintel Nov 21, 2023

kintel Nov 21, 2023

kintel Nov 21, 2023

kintel commented Nov 21, 2023

Speed up script evaluation by prehashing & interning identifier strings #4648

Are you sure you want to change the base?

Speed up script evaluation by prehashing & interning identifier strings #4648

Conversation

ochafik commented May 23, 2023 • edited

jordanbrown0 commented May 23, 2023

ochafik commented May 23, 2023

kintel Jun 30, 2023

Choose a reason for hiding this comment

kintel Jun 30, 2023

Choose a reason for hiding this comment

ochafik Nov 21, 2023

Choose a reason for hiding this comment

kintel Nov 21, 2023

Choose a reason for hiding this comment

kintel Nov 21, 2023

Choose a reason for hiding this comment

kintel Nov 21, 2023

Choose a reason for hiding this comment

kintel commented Nov 21, 2023

ochafik commented May 23, 2023 •

edited