I have optimized the code #32
Conversation
Added Go Impl.
Create shortUrl.go
…g object everytime, rely on strings.builder for doing the work on the given object. The builder does not provide a way to insert in front, so we have to reverse the whole thing later.
|
Thanks! Could you perhaps just add the chars in the reverse order again, instead of requiring a whole additional function Moreover, what’s And, again, are the two |
|
The string builder and byte builder do NOT give access to the internal array to push in reverse order. So, I have to reverse it after the string is built from it. In my benchmarking, I found that this may be taking 30-40% of total CPU when it comes to Encode(). I will continue to search for better options, but overall this solution is 55% faster than the original where I was using "string" in for loop. |
|
I have removed reverseRunes. The idea was to say that if they go and put
Unicode in Alphabet list, then they have to switch to reverseRunes()
I have made the 2 functions independent of codec block.
Also, I did study strings. builder writers and byte array writers, but they
do not give control to write in reverse into internal array.
Best Regards,
Sandeep Kalra
…On Thu, Jun 14, 2018 at 2:56 PM Marco ***@***.***> wrote:
Thanks!
Could you perhaps just add the chars in the reverse order again, instead
of requiring a whole additional function reverseChars?
Moreover, what’s reverseRunes? It doesn’t seem to be used at all.
And, again, are the two Codec blocks necessary?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#32 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFTvwpstRQrSdw-X12o0bJBY5X3RnQ5wks5t8r_jgaJpZM4UoXVV>
.
|
Go/shortUrl.go
Outdated
| i := strings.Index(Alphabets, string(c)) | ||
| if i < 0 { | ||
| return 0, fmt.Errorf("Invalid character %s in input %s", string(c), path) | ||
| } else { |
There was a problem hiding this comment.
There is no need for else, also it is not idiomatic from what I know.
Go/shortUrl.go
Outdated
| func Decode(path string) (int, error) { | ||
| n := 0 | ||
| for _, c := range path { | ||
| i := strings.Index(Alphabets, string(c)) |
There was a problem hiding this comment.
I think you can use strings.IndexRune, you already have the rune as c, this way you can get rid of the (string) cast.
Go/shortUrl.go
Outdated
| * + proof against offensive words (removed 'a', 'e', 'i', 'o' and 'u') | ||
| * + unambiguous (removed 'I', 'l', '1', 'O' and '0') | ||
| **/ | ||
| package ShortURL |
|
To write in reverse I think the only way is to use a normal slice of Runes, based on the specific usage of the function (we have a limited, known set of runes) we should not have any problem (ones that are fixed by bytes.buffer and strings.builder). I think if the performance gained is significant it worth the change (from builder to slice). The Grow formula is the same (2*n), for the builder internal slice and for a regular slice, so memory allocation should no differ, but we will eliminate 3 casts (string, bytes, string) and n/2 swap operations (from the reverse). |
Go/shortUrl.go
Outdated
| const ( | ||
| Alphabets = "23456789bcdfghjkmnpqrstvwxyzBCDFGHJKLMNPQRSTVWXYZ-_" | ||
| Base = len(Alphabets) | ||
| ) |
There was a problem hiding this comment.
Do we need to export Alphabets and Base?
There was a problem hiding this comment.
All the other implementations (at least PHP,Java) have them public, so for the compatibility sake so Go too?
I would imagine it would be nice to have them as public, maybe some web servers need to do some white listing, or dynamic routing based on them?
…de for alternate implementation and the corresponding benchmark tests
|
As for the test you can use 2 other implementations with some random numbers, and use those as a test in the Go version. I am surprised that no other implementation has tests, so Go must be better. |
New Code uses strings builder to do at least 50% faster processing.