Hash based lookup implementation #143
Conversation
guliyevemil1
force-pushed
the
master
branch
2 times, most recently
from
22d5185 to
53ad4a6
Compare
|
If you want me to add comments to explain the logic a bit, let me know. I basically took what you were doing with regexes and just did it via string manipulation and this made it much faster. |
|
Btw, there was a small bug that's kind of a gotcha in Go if you haven't seen it before. The logic for figuring out |
|
Hey, can I get a timeline of when you think you can merge this? I'd like to use your library but this is a blocker. Thanks! |
|
I need to allocate some time to look into it. I will try to find some time within this week. |
|
Hey man, I understand you're busy. I found your library from the official publicsuffix.org list and I liked your API better than x/net package. The performance was unacceptable for my use case so I decided to help you and improve it. It passes all your tests and the benchmarks are comparable or better than x/net (which I added in the first commit). Can you please spare some time soon to merge this? I would like to continue using your library instead of being forced to use my forked off version that I don't think I can maintain. |
|
There is a subtle but important implication in this change, and I want to add a note here mostly as a reminder. This change will assume that For better or for worse, this is the same current limitation of Depending on the outcome of the discussion, the index key may have to be reshaped. |
There was a problem hiding this comment.
Thanks @guliyevemil1. I approve these changes, and I'll merge them.
Before going ahead, can you clarify the intent of the change in the DefaultList.rules assignment?
| @@ -39,7 +39,9 @@ func init() { | |||
| { {{$r.Type}}, "{{$r.Value}}", {{$r.Length}}, {{$r.Private}} }, | |||
| {{end}} | |||
| } | |||
| DefaultList.rules = r[:] | |||
| for i := range r { | |||
There was a problem hiding this comment.
What's the reason of this change? Is it because of #141?
| if !rule.Match(name) { | ||
| continue | ||
| for { | ||
| rule, ok := l.rules[name] |
There was a problem hiding this comment.
There is a subtle but important implication in this change, and I want to add a note here mostly as a reminder.
This change will assume that *.foo and foo won't exist in the same definition.
Please see the main ticket for further information.
| @@ -8626,5 +8626,7 @@ func init() { | |||
| {1, "now.sh", 2, true}, | |||
| {1, "zone.id", 2, true}, | |||
| } | |||
| DefaultList.rules = r[:] | |||
| for i := range r { | |||
| DefaultList.AddRule(&r[i]) | |||
There was a problem hiding this comment.
What's the reason of this change? Is it because of #141?
There was a problem hiding this comment.
OK, nevermind. I actually figured it out, we need to pass via AddRule because the internal rule is no longer a list.
My bad, sorry for the stupid question.
I need to determine the cost of this operation. It could be possible that it's more efficient to compile directly the r in the appropriate form and then assign.
There was a problem hiding this comment.
I can't speak with absolute certainty, but my gut feeling is that this implementation eases the load on the garbage collector because nothing in this list should ever be garbage collected which means it's better to have all of them in a single giant array rather than allocating memory for each individually on the heap. I might be wrong and it might be worth benchmarking.
weppos
changed the title
This is an edge case resulting from #143. Once again, the match of wildcard rules is tricky as I also mentioned in #143 (comment)
|
Thank you so much for taking the time to review the change and merging it! |
No description provided.