use a strings.Replacer to reverse names in internal/abi #892
Conversation
|
Here's my initial stab at a less naive algorithm. |
|
This is a lot of code even if its lifted from std. Can you time how long this actually takes with a huge struct and compare before after? I did some napkin math and think the before is fine even for comically large structs. |
|
If this is actually beneficial is there a way import/ linkname/ insert this code from std instead of having it in this repo? |
|
Fair enough, I'll come back with some benchmarks. We can't reuse strings.Replacer directly, no. For one, it uses reflection, which I explain in the comment at the top. And also, we can't assume that a main package already depends on |
This way, rather than using a double loop quadratic algorithm
to search for each name to replace in a string,
we can make use of the reasonably efficient generic replacer
which makes use of tries.
Copying some code from the strings package is not ideal,
but it beats having to re-implement such an algorithm ourselves.
Not only is the algorithm much faster, as we are no longer quadratic,
but the replacer also appends into a buffer to avoid repeated string
copies, which means we allocate fewer bytes per operation.
│ old │ new │
│ sec/op │ sec/op vs base │
AbiOriginalNames-8 135708.0n ± 0% 391.1n ± 5% -99.71% (p=0.001 n=7)
│ old │ new │
│ B/s │ B/s vs base │
AbiOriginalNames-8 2.565Mi ± 0% 890.112Mi ± 4% +34597.03% (p=0.001 n=7)
│ old │ new │
│ B/op │ B/op vs base │
AbiOriginalNames-8 5464.0 ± 0% 848.0 ± 0% -84.48% (p=0.001 n=7)
│ old │ new │
│ allocs/op │ allocs/op vs base │
AbiOriginalNames-8 18.00 ± 0% 16.00 ± 0% -11.11% (p=0.001 n=7)
|
The speedup, as measured, is over 200x. So yes, I still think this is worth adding. Plus, we allocate less too, which is also rather important for such a low level API. |
Now that we only use the list to create a replacer at init time, we no longer need to spend extra effort sorting by length first. The benchmark shows no measurable difference in performance.
(see commit message)