Skip to content

Conversation

@jpcaruana
Copy link
Collaborator

@jpcaruana jpcaruana commented Jan 31, 2020

Hi @MickaelBergem,

as I was reading through the code, I realized that checking for blacklisted endpoint could be tedious as the list grows.

I tested it

if __name__ == "__main__":
    b = ['/jobs/*/results', '/jobs/*/*/progress', ]
    times = timeit.Timer(functools.partial(is_in_blacklist, '/test', b)).repeat(repeat=3, number=1000)
    print(times)

    b = b + b + b + b + b + b
    b = b + b + b + b + b + b
    b = b + b + b + b + b + b
    b = b + b + b + b + b + b
    b = b + b + b + b + b + b

    times = timeit.Timer(functools.partial(is_in_blacklist, '/test', b)).repeat(repeat=3, number=1000)
    print(times)

the result was obvious, the more endpoint you add, the longer it takes:

short list: [0.004838313005166128, 0.004496372013818473, 0.004295461985748261]
long list:  [30.687488282012055, 33.16138723300537, 27.81307909899624]

with the code from this PR, the result is better even for a short list:

short list: [0.001244339015102014, 0.0012035770050715655, 0.0014499509998131543]
long list:  [4.755746807000833, 4.928609041991876, 4.693075067014433]

With this PR, you pay the re.compiile prize once at the init of HowFastFlaskMiddleware - I don't know if it is worth it.

Copy link
Collaborator

@MickaelBergem MickaelBergem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That performance improvement looks amazing, thanks a lot for the contribution!

Two ideas I had reading this PR (nothing immediately important):

  • we could probably support passing regexps in the blacklist if some users need it, in the future
  • once the library grows, I would be curious to explore what are the best practices around running performance benchmarks in CI

Also, regarding your note on the price of the initial compile, I assume this would only have an impact in serverless environments where the entire Flask application would be initialized often (and still), so it looks like a real win for me given the gains you mentioned.

Feel free to merge as soon as you want :)

@jpcaruana jpcaruana merged commit cfce556 into master Feb 3, 2020
@jpcaruana jpcaruana deleted the optimize_endpoint_blacklist branch February 3, 2020 07:58
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.

3 participants