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

Replace _val with separate props for url parts #172

Closed
asvetlov opened this issue Feb 18, 2018 · 5 comments · Fixed by #1397
Closed

Replace _val with separate props for url parts #172

asvetlov opened this issue Feb 18, 2018 · 5 comments · Fixed by #1397

Comments

@asvetlov
Copy link
Member

The target is URL.build optimization.
The method is crucial for aiohttp web server, avoiding SplitResult construction can make the method faster.

SplitResult could be restored in every method that need the object.

N.B. Serialization format should support URL classes generated by older library version or the format should not be changed at all (the later looks easier).

@bdraco
Copy link
Member

bdraco commented Oct 30, 2024

#1396 will get a step closer to this

In the future we may want to change the split functions to unpack the values into named variables in future. ie self._scheme, self._netloc, self._path, self._query, self._fragment = val or store everything in self._cache as the single source of truth; However that was too large of a redesign for this PR since it would also require changing all the tuple unpacking and pre cache in __new__.

One consideration about getting rid of the _val tuple is that SplitResult is exposed as an allowed incoming value in new we would have to have code to convert it. With _val being a normal tuple, SplitResult can be passed in and its still compatible.

It would be a lot nicer if we stored everything in self._cache though since it means the encode_url and pre_encoded_url calls would return a simple dict that was pre-populated, and it also means propcache would have more keys pre-populated.

@bdraco

This comment was marked as outdated.

@bdraco bdraco closed this as completed Oct 30, 2024
@bdraco bdraco reopened this Oct 30, 2024
@bdraco
Copy link
Member

bdraco commented Oct 30, 2024

Going to leave it open until we get a +/- on performance on #1397 to see if we can move the 2nd attempt forward

@bdraco
Copy link
Member

bdraco commented Oct 30, 2024

Second attempt in #1397 looks like it will work without regressing performance in a meaningful way

@bdraco
Copy link
Member

bdraco commented Oct 30, 2024

I was able to squeeze more out of and keep it readable in #1397
Glad I stuck with it

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 a pull request may close this issue.

2 participants