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

Adding CROSSORIGIN & django-csp handling #405

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

karolyi
Copy link
Contributor

@karolyi karolyi commented Jul 28, 2024

The crossorigin tag is necessary for cases when the loaded assets have their integrity tag set and the origin of theirs are different from that of the loading page.

Besides, there's a couple improvements in the code as well.

@karolyi karolyi changed the title Adding CROSSORIGIN handling Adding CROSSORIGIN & django-csp handling Jul 28, 2024
@karolyi
Copy link
Contributor Author

karolyi commented Jul 29, 2024

I have more ideas about immensely speeding up this module with utilizing caches, but this'll have to suffice for now.

loader_class = import_string(config['LOADER_CLASS'])
_loaders[config_name] = loader_class(config_name, config)
return _loaders[config_name]
@lru_cache(maxsize=None)
Copy link
Member

Choose a reason for hiding this comment

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

This is changing the API w/o a clear benefit. While _loaders is private, someone might be using it.
I would refrain from changing this unless there's a very clear benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 'very clear benefit' is to speed up config loading immensely, because template generation runs this function countless times when generating a single template, by looking at the various code paths.

_loader is a name that implies you shouldn't use that variable outside of the module, and it's everybody's responsibility to do just that, or face the consequences if they don't.

Copy link
Member

Choose a reason for hiding this comment

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

The function already does the same thing lru_cache will do. _loaders is exactly the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me demonstrate you why reaching for a global variable is slower than using lru_cache that uses a cache table locally:

import timeit
from functools import lru_cache
from random import randint

_some_global_variable = {}


def expensive_function(var1):
    return randint(0, 5)


def get1(var1):
    if var1 not in _some_global_variable:
        result = expensive_function(var1)
        _some_global_variable[var1] = result
        return result
    return _some_global_variable[var1]


@lru_cache(maxsize=None)
def get2(var1):
    return expensive_function(var1)


print(timeit.timeit(
    stmt='get1("x")', setup='from __main__ import get1', number=10000000))
print(timeit.timeit(
    stmt='get2("x")', setup='from __main__ import get2', number=10000000))

A couple run outputs with various python versions I have currently installed:

sh-5.2$ python3.8 test.py
0.7265332540009695
0.3478194800009078

sh-5.2$ python3.9 test.py
0.6371905300002254
0.3206007739972847

sh-5.2$ python3.10 test.py
0.48459655799888424
0.2498635900010413

sh-5.2$ python3.11 test.py
0.48944291500083636
0.38999983699977747

sh-5.2$ python3.12 test.py
0.425895505999506
0.3634855400014203

Copy link
Member

@fjsj fjsj left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but we need more tests for the new code, including error conditions.

@karolyi
Copy link
Contributor Author

karolyi commented Aug 2, 2024

Hey,

I've already provided test code that test the changes out, and using my fork in a pretty huge project, which was the reason to make the contribution in the first place.

Feel free to add those tests if you want to, this should be good to go either way.

@karolyi
Copy link
Contributor Author

karolyi commented Nov 9, 2024

bumperino

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.

2 participants