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

Short-circuit primitive encoding on CoercingEncoder #7655

Merged
merged 7 commits into from May 7, 2019

Conversation

Projects
None yet
5 participants
@blorente
Copy link
Contributor

commented May 3, 2019

Problem

CoercingEncoder.encode() is called many times when fingerprinting options. There is a significant overhead to the isinstance() calls in python reference. This means that, when datatypes recursively encode themselves after #7304, they do twice the checks that they already did, causing a several-second regression in large targets (from 56s to 59s, without pantsd).

Solution

Short-circuit primitives to avoid some of those checks.

There is probably more reordering of branches to be done, but I don't know enough about the callers of that function to reorder safely, since the if-else branches return early and thus the cases might not be a real partition of the set of possible inputs.

Result

The noop compile times of that large target are back to 56s.

Acknowledgements

@ity and @stuhood for finding the regressing commit, and @cosmicexplorer and @illicitonion for helping find the root cause.

@illicitonion
Copy link
Contributor

left a comment

Thanks!

def default(self, o):
if self._is_primitive(o):
# isinstance() checks are expensive, particularly isinstance(o, Mapping):

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 3, 2019

Contributor
Suggested change
# isinstance() checks are expensive, particularly isinstance(o, Mapping):
# isinstance() checks are expensive, particularly for abstract base classes such as Mapping:
logger.debug("Our custom Encoder is trying to hash a primitive type, but has gone through"
"checking every other data type before. These checks are expensive,"
"so you should consider adding the type of your primitive {} to the top"
"of this function".format(type(o)))

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 3, 2019

Contributor
Suggested change
"of this function".format(type(o)))
"of this function (CoercingEncoder.default).".format(type(o)))
@stuhood

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Thanks for the investigation!

I'm not sure how much appetite you have for a deeper fix, but looking a little higher in the code, I see the following issue:

diff --git a/src/python/pants/option/options_fingerprinter.py b/src/python/pants/option/options_fingerprinter.py
index 1309993..8ea1020 100644
--- a/src/python/pants/option/options_fingerprinter.py
+++ b/src/python/pants/option/options_fingerprinter.py
@@ -71,6 +71,8 @@ class OptionsFingerprinter(object):
     if option_val is None:
       return None

+    print('>>> fingerprinting {}: {}'.format(option_type, option_val))
+
     # Wrapping all other values in a list here allows us to easily handle single-valued and
     # list-valued options uniformly. For non-list-valued options, this will be a singleton list
     # (with the exception of dict, which is not modified). This dict exception works because we do

... shows that we are repeatedly refingerprinting the same options a bunch of times. This appears to be due to the OptionsFingerprinter class:

  1. not having any sort of caching or memoization
  2. not being reused across multiple requests

So, it's possible that a deeper fix for this issue would either be to: a) add memoization to OptionsFingerprinter and reuse it (possibly by putting it in the Context?), b) move options fingerprinting into the Options container, so that it is memoized there.

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I will create a tracking issue for that, because if we're going to find a deeper fix, I'd rather investigate why we call this hashing that often to begin with, and this fix would be a perf win regardless.

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Opened #7658 to track that.

blorente added some commits May 3, 2019

@stuhood

stuhood approved these changes May 3, 2019

Copy link
Member

left a comment

Thanks!

@Eric-Arellano
Copy link
Contributor

left a comment

Looks great! I like this rename.

@stuhood stuhood added this to the 1.15.x milestone May 3, 2019

Show resolved Hide resolved src/python/pants/base/hash_utils.py
Show resolved Hide resolved src/python/pants/base/hash_utils.py Outdated
Show resolved Hide resolved src/python/pants/base/hash_utils.py Outdated
else:
logger.debug("Our custom Encoder is trying to hash a primitive type, but has gone through"
"checking every other data type before. These checks are expensive,"
"so you should consider adding the type of your primitive {} to the top"

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 4, 2019

Contributor
Suggested change
"so you should consider adding the type of your primitive {} to the top"
"so you should consider registering the type of the class {} within"
Show resolved Hide resolved src/python/pants/base/hash_utils.py Outdated
@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

One possible addition to this approach is to employ a json hash cache for option fingerprinting. This has the additional benefit of not producing a potentially large number of debug logs whenever a user attempts to encode one of their own classes. It wouldn't solve the problem that this change fixes (which should definitely still be fixed), but it does potentially address the problem raised by the debug log.

Right now, if a user creates their own custom option type outside of the pants repo and tries to use it, they'll see a lot of debug logs for an issue they can't fix without making a change in upstream pants. If we take this route, I would prefer that it be possible to add a custom branch to the default method in a way that plugins can modify during options fingerprinting.

@blorente

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Right now, if a user creates their own custom option type outside of the pants repo and tries to use it, they'll see a lot of debug logs for an issue they can't fix without making a change in upstream pants.

I agree that, for a plugin author, this might be frustrating. I would prefer to apply your suggestions, remove the log statement and land this regardless, while we look at the root cause of "why we are parsing options so many times" (which is the problem the cache would ameliorate) as part of #7658.

Removing the log statement is not worse than what we had before, because we still had this inefficiency and no log to show it, and it is a perf win with no drawbacks.

@stuhood stuhood merged commit 8615b15 into pantsbuild:master May 7, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.