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

Lots of integer variables in func stable and func merge #1

Open
nsajko opened this issue May 3, 2018 · 2 comments
Open

Lots of integer variables in func stable and func merge #1

nsajko opened this issue May 3, 2018 · 2 comments

Comments

@nsajko
Copy link
Owner

nsajko commented May 3, 2018

@vdobler

I still did not quite get what exactly do you want regarding the "opaque struct" (previous discussion here: [0], CL: [1]). What variables would go in the struct and what code from func stable should go in its methods? I think you may have imagined a struct with very many methods, and I fear that that would increase code size substantially. For example, do you know that even before calling func merge in func stable we have to check if block indices coincide with BDS (block distribution storage) or MIB (movement imitation buffer) indices (so as to restrict the blocks to their proper range before merging, without including BDS or MIB) [2]?

I do see that it would probably be better to pass references to structs to func merge instead of all those integer variables.

On the other hand there is a related change possible that I only did not include before because of Go doing little inlining: the code in func stable from [3] to [4] could be separated out into a function. That would eliminate the identifiers/variables bds0, bds1, backupBDS0, backupBDS1 from func stable; that is why I think you would like it ...?

Sidenote: the future changes to my code should probably happen here on branches other than master instead of on the Go Gerrit instance because of rsc being annoyed by the amount of patchsets.

[0] golang/go#25137
[1] https://go-review.googlesource.com/c/go/+/101415
[2] https://go-review.googlesource.com/c/go/+/101415/41/src/sort/sort.go#1246
[3] https://go-review.googlesource.com/c/go/+/101415/41/src/sort/sort.go#1009
[4] https://go-review.googlesource.com/c/go/+/101415/41/src/sort/sort.go#1170

@nsajko nsajko changed the title Lots of integer variables in of func stable and func merge Lots of integer variables in func stable and func merge May 3, 2018
@nsajko
Copy link
Owner Author

nsajko commented May 4, 2018

@vdobler

Can I just get feedback on the idea to separate code from func stable into a different function, before other things?

@vdobler
Copy link
Contributor

vdobler commented May 5, 2018

Of course your code mus be seperated into much much smaller functions and these function do need tests.

nsajko pushed a commit that referenced this issue Mar 25, 2019
Most chars in URLs are lowercase, so check that first.

Performance change:

String-4               7.62µs ± 1%    7.27µs ± 3%  -4.61%  (p=0.008 n=5+5)
QueryEscape/#00-4      92.6ns ± 3%    90.3ns ± 1%  -2.48%  (p=0.016 n=5+5)
QueryEscape/#1-4       515ns ± 4%     510ns ± 2%    ~     (p=0.683 n=5+5)
QueryEscape/#2-4       375ns ± 1%     343ns ± 1%  -8.52%  (p=0.008 n=5+5)
QueryEscape/#3-4       758ns ± 1%     699ns ± 1%  -7.83%  (p=0.008 n=5+5)
QueryEscape/#04-4      6.06µs ± 1%    5.74µs ± 1%  -5.38%  (p=0.008 n=5+5)
PathEscape/#00-4        140ns ± 1%     135ns ± 2%  -3.85%  (p=0.008 n=5+5)
PathEscape/#1-4        511ns ± 3%     507ns ± 3%    ~     (p=0.587 n=5+5)
PathEscape/#2-4        372ns ± 1%     342ns ± 2%  -8.22%  (p=0.008 n=5+5)
PathEscape/#3-4        747ns ± 1%     685ns ± 1%  -8.30%  (p=0.008 n=5+5)
PathEscape/#04-4       5.94µs ± 1%    5.64µs ± 3%  -4.98%  (p=0.008 n=5+5)
QueryUnescape/#00-4     111ns ± 4%     110ns ± 2%    ~     (p=0.952 n=5+5)
QueryUnescape/#1-4     390ns ± 0%     391ns ± 2%    ~     (p=0.714 n=5+5)
QueryUnescape/#2-4     297ns ± 5%     295ns ± 3%    ~     (p=0.524 n=5+5)
QueryUnescape/#3-4     543ns ± 3%     556ns ± 2%  +2.39%  (p=0.032 n=5+5)
QueryUnescape/#04-4    3.23µs ± 3%    3.22µs ± 2%    ~     (p=1.000 n=5+5)
PathUnescape/#00-4      111ns ± 1%     110ns ± 3%    ~     (p=0.881 n=5+5)
PathUnescape/#1-4      389ns ± 2%     386ns ± 2%    ~     (p=0.444 n=5+5)
PathUnescape/#2-4      297ns ± 1%     295ns ± 3%    ~     (p=0.738 n=5+5)
PathUnescape/#3-4      557ns ± 3%     553ns ± 2%    ~     (p=0.810 n=5+5)
PathUnescape/#04-4     2.94µs ± 2%    2.97µs ± 2%    ~     (p=0.222 n=5+5)

Change-Id: I7e6d64cd5f8f5218cb40f52f0015168a8674aabb
Reviewed-on: https://go-review.googlesource.com/c/go/+/168883
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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

No branches or pull requests

2 participants