Repository navigation
Cache plugin configs - #3656
Cache plugin configs#3656peterebden wants to merge 5 commits into
Conversation
| case *pyRange: | ||
| return name == "range" | ||
| case pyList: | ||
| case pyList, pyFrozenList: |
There was a problem hiding this comment.
this is not strictly related but a bit of a drive-by while I was here; isinstance should treat a frozen list as being a list
| panic("unknown dict key: " + s.String()) | ||
| case Union: | ||
| d2, ok := operand.(pyDict) | ||
| d2, ok := asDict(operand) |
There was a problem hiding this comment.
another drive-by about using a union operator of a dict and a frozen dict
| } | ||
| defer f.Close() | ||
| defer pprof.WriteHeapProfile(f) | ||
| defer runtime.GC() |
There was a problem hiding this comment.
and here; we only get up-to-date stats for the heap profile once a GC cycle has completed, without this that might have been ages ago
| case pyList, pyFrozenList: | ||
| return name == "list" | ||
| case pyDict: | ||
| case pyDict, pyFrozenDict: |
There was a problem hiding this comment.
I ran a quick check against my repo and noticed differences in output, I suspect a change in behaviour; top-level variables in a sub included .build_defs file are frozen once it has loaded, so isinstance(x, dict) and isinstance(x, list) now return True where they used to return False.
This changed output (ran a plz query print comparison with plz-before and plz-after), it's probably worth noting the slight change in behaviour (which probably makes more sense).
08:47:24.702 WARNING: BUILD: describe()=plain value, isinstance(_DEFAULT_TAGS, list)=False
vs
08:47:32.650 WARNING: BUILD: describe()=per-env dict, isinstance(_DEFAULT_TAGS, list)=True
on
$ cat /tmp/isinstance-test0/defs/defs.build_defs
_UNSET = {"unset": True} # sentinel default
_DEFAULT_TAGS = ["base"]
def describe(setting = _UNSET):
if isinstance(setting, dict):
return "per-env dict"
return "plain value"
def tags_is_list():
return isinstance(_DEFAULT_TAGS, list)
would it make sense to check val != _UNSET before the isinstance call?
There was a problem hiding this comment.
Yes exactly; there are some fixes in #3657 which I'll land in the same release as this and I've landed a fix internally to deal with that _UNSET thing so we should be fine before and after the upgrade.
Thanks for looking into it!
Turns out we spend a lot of time rebuilding config objects for every plugin / package combination, which really adds up time-wise. This implements caching for those config objects, which are usually identical.
On this machine, with a test repo generated from the perf tooling, this saves about 10% of wall clock time, 20% of user CPU and 27% of peak memory allocation, so it's pretty significant.