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

caching of checks for whether to log at DEBUG2 levels #167

Merged

Conversation

@pipermerriam
Copy link
Member

commented Aug 5, 2019

What was wrong?

DEBUG2 implements some performance issues with the overhead of determining if logs should be shown for a given level each time the function is called.

How was it fixed?

We fully cache whether the log level is enabled as well as turning the ExtendedDebugLogger.debug2 function into a noop in the event that DEBUG2 is disabled.

Cute Animal Picture

13-07-13 Red Sport (11)C750R

@pipermerriam pipermerriam requested a review from carver Aug 5, 2019

@pipermerriam pipermerriam force-pushed the pipermerriam:piper/cache-whether-debug2-is-shown branch from bf17f25 to c0150ed Aug 5, 2019

# When we find that `DEBUG2` isn't enabled we completely replace
# the `debug2` function in this instance of the logger with a noop
# lambda to further speed up
self.__dict__['debug2'] = lambda message, *args, **kwargs: None

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 5, 2019

Author Member

@carver this seemed superior since it should further reduce the overhead of calling the function.

@carver

carver approved these changes Aug 5, 2019


from .toolz import assoc

DEBUG2_LEVEL_NUM = 8


class cached_show_debug2_property:

This comment has been minimized.

Copy link
@carver

carver Aug 5, 2019

Collaborator

Apart from typing, this looks totally generic; basically the same as @cached_property. Are you just trying to avoid the dependency?

If so, maybe we could just make this class into an internal cached_property implementation in eth-utils. Seems like we'd want to reuse it elsewhere.

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 5, 2019

Author Member

Yes, avoiding the dependency. For libraries that really need it I have no issue adding the dependency but this is a somewhat small and specialized case and it allows us to maintain good type hints.

assert 'debug2' not in logger.__dict__
assert logger.debug2('this should actually call the function') is None
assert 'debug2' in logger.__dict__
assert logger.debug2('should not do anything but hit the lambda') is None

This comment has been minimized.

Copy link
@carver

carver Aug 5, 2019

Collaborator

I can't see anywhere that the test is making sure that the log is being dropped.

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 5, 2019

Author Member

I'll add a caplog check, forgot I could do that.

assert logger.debug2('should not do anything but hit the lambda') is None

# now see that it always returns the value from `__dict__`
logger.__dict__['show_debug2'] = 100

This comment has been minimized.

Copy link
@carver

carver Aug 5, 2019

Collaborator

I didn't follow this test. Doesn't it kind of do the same thing as line 47?

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 5, 2019

Author Member

Yes, still just showing that it caches the value and pulls it from __dict__. Somewhat weak test but I wanted to flex it in both situations where debug is enabled or not.

assert 'debug2' not in logger.__dict__
assert logger.debug2('this should actually call the function') is None
assert 'debug2' not in logger.__dict__
assert logger.debug2('this should still call the function') is None

This comment has been minimized.

Copy link
@carver

carver Aug 5, 2019

Collaborator

Any way to check that the log is actually being emitted?

This comment has been minimized.

Copy link
@pipermerriam

pipermerriam Aug 5, 2019

Author Member

Yeah, gonna add a caplog test.

@pipermerriam pipermerriam merged commit b6e7fa7 into ethereum:master Aug 5, 2019

8 checks passed

ci/circleci: doctest Your tests passed on CircleCI!
Details
ci/circleci: lint-py36 Your tests passed on CircleCI!
Details
ci/circleci: lint-py37 Your tests passed on CircleCI!
Details
ci/circleci: py35 Your tests passed on CircleCI!
Details
ci/circleci: py36 Your tests passed on CircleCI!
Details
ci/circleci: py36-typing1 Your tests passed on CircleCI!
Details
ci/circleci: py37 Your tests passed on CircleCI!
Details
ci/circleci: pypy3 Your tests passed on CircleCI!
Details

@pipermerriam pipermerriam deleted the pipermerriam:piper/cache-whether-debug2-is-shown branch Aug 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.