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

Using indexOf() instead of Regexp() #122

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
2 participants
@gorhill
Contributor

gorhill commented Oct 25, 2013

Performance improvement seen at (along with the intermediate experiments using binary search):
http://jsperf.com/uri-js-sld-regex-vs-binary-search/4

Memory footprint on my 32-bit OS:
Regexp: > 500K
indexOf: < 20K

A test has been added to ensure all SLD are correctly processed by the new code.

@gorhill

This comment has been minimized.

Show comment
Hide comment
@gorhill

gorhill Apr 13, 2014

Contributor

Moving on.

Contributor

gorhill commented Apr 13, 2014

Moving on.

@gorhill gorhill closed this Apr 13, 2014

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Apr 13, 2014

Member

Why did you close the PR? I've only not merged this PR because I wasn't sure if I'd want to go with 1.13 or 2.0 and fix some other things as well. Meanwhile it's clear that 1.13 will have to come first, as 2.0 is a longer way out. Please reopen the PR.

Member

rodneyrehm commented Apr 13, 2014

Why did you close the PR? I've only not merged this PR because I wasn't sure if I'd want to go with 1.13 or 2.0 and fix some other things as well. Meanwhile it's clear that 1.13 will have to come first, as 2.0 is a longer way out. Please reopen the PR.

@gorhill

This comment has been minimized.

Show comment
Hide comment
@gorhill

gorhill Apr 19, 2014

Contributor

I was wondering if you would be interested in the ability to support the full Public Suffix List? I think it's not too much work to fit in there publicsuffixlist.js.

User could optionally supply his own list, while the default (stock) list (if no external list supplied) would be a match of what SecondLevelDomain currently handles.

Contributor

gorhill commented Apr 19, 2014

I was wondering if you would be interested in the ability to support the full Public Suffix List? I think it's not too much work to fit in there publicsuffixlist.js.

User could optionally supply his own list, while the default (stock) list (if no external list supplied) would be a match of what SecondLevelDomain currently handles.

@rodneyrehm

This comment has been minimized.

Show comment
Hide comment
@rodneyrehm

rodneyrehm Apr 19, 2014

Member

I would prefer using PublicSuffix as it used by browsers and maintained properly. I haven't used PS because it also contains entries like github.io, which are not SLDs, but used by the browser for cookie origin control.

If your list filters these out, we can talk about depending on or integrating your implementation (or a derivation thereof). If it doesn't, don't see how this could work.

btw. this should probably be a separate issue. care to open one?

Member

rodneyrehm commented Apr 19, 2014

I would prefer using PublicSuffix as it used by browsers and maintained properly. I haven't used PS because it also contains entries like github.io, which are not SLDs, but used by the browser for cookie origin control.

If your list filters these out, we can talk about depending on or integrating your implementation (or a derivation thereof). If it doesn't, don't see how this could work.

btw. this should probably be a separate issue. care to open one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment