Skip to content

Conversation

@hhaensel
Copy link
Contributor

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
Contributor 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)

@hhaensel
Copy link
Contributor Author

@twavv I think this PR is still valid!
AssetRegistry deserves a Project.toml plus a normpath during key generation, plus some minor performance issues.

I've brushed up the PR to be compatible with the latest JSON v1 and the test now also pass with Julia 1.12

@hhaensel
Copy link
Contributor Author

@shashi , @JobJob, @barche Not sure, who's currently mainting AssetRegistry. Pinging you as this PR is a prerequisite to update WebIO to work with JSON v1

@barche
Copy link
Contributor

barche commented Oct 17, 2025

I have no idea, sorry.

@shashi
Copy link
Member

shashi commented Oct 17, 2025

@hhaensel hey I messaged you on Slack! I'm not sure if I can just hit merge here, yet, but willing to verify this with you.

Project.toml Outdated
@@ -0,0 +1,20 @@
name = "AssetRegistry"
uuid = "bf4720bc-e11a-5d0c-854e-bdca1663c893"
version = "0.1.0"
Copy link
Member

Choose a reason for hiding this comment

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

This should definitely be a higher version because v0.1.0 is currently released. It should be v0.2.0, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is non-breaking it could as well be 0.1.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my projects I tend to make a separate commit for the version change as that is better visible in the git tree

@hhaensel
Copy link
Contributor Author

hhaensel commented Oct 18, 2025

Added a changelog and set version 0.1.1.
Should we consider #16 before tagging? Maybe @Nosferican can comment

EDIT: in that case I'd remove the last commit and add that after including #16

Project.toml Outdated
[compat]
JSON = "0.0.0 - 0.21, 1"
Pidfile = "1"
julia = "1" No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Correct me if I'm wrong, updating the compat of Julia means it's a breaking release, so usually the version bump should reflect that. 0.1.1 means that we cannot patch 0.1 for 0.7 only. Which I think is ok but just trying to use the semantic thing here because sometimes the reviewers will complain.

@shashi shashi merged commit f604967 into JuliaGizmos:master Oct 20, 2025
@hhaensel
Copy link
Contributor Author

I think, we could have kept 0.1.1, or kept compat with julia 0.7. Anyhow the usage of 0.7 is not encouraged, so that wouldn't hurt anyone.

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.

4 participants