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

Add Project.toml, speedup of isregistered and getkey, path consistency #14

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

Conversation

hhaensel
Copy link

During my work at JuliaGizmos/WebIO.jl#417, I realised some (minor) performance issues with getkey(), which I thought was due to the recalculation of the key.

Only later I realised that the main bottle neck - at least on my windows machine - is from abspath(). Nevertheless, I propose a PR here to change getkey such that it checks for the values of the registry instead of the keys, and at the same time bring AssetRegistry to the new package format.

@twavv
Copy link
Member

twavv commented Sep 13, 2021

It's been a minute since you made this PR (sorry!). Are you still interested in having it merged?

I don't understand why this change is being made. Why would we prefer ... in values(...) over haskey?

@hhaensel
Copy link
Author

It took some time for me to remember 😉
First of all, it is not a very important point, as abspath() is taking most of the time.

haskey() is not the point but the calculation of sha1, everytime that you call getkey(). If the paths are long you might gain some speed in searching for the keys instead of the values. But the potential gain in speed is probably not worth the complication in code.

I think the most important about this PR in the end is the usage of normpath(). It will get rid of . and .. which caused trouble for me, if I remember correctly.

So why not making the following change?

- getkey(path) =  baseurl[] * "/assetserver/" * bytes2hex(sha1(abspath(path))) * "-" * basename(path)
+ getkey(path) =  baseurl[] * "/assetserver/" * bytes2hex(sha1(normpath(abspath(path)))) * "-" * basename(path)

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