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

Pure Ruby masking #48

Merged
merged 2 commits into from
Dec 27, 2016
Merged

Pure Ruby masking #48

merged 2 commits into from
Dec 27, 2016

Conversation

kbarrette
Copy link
Contributor

I was having some trouble with Windows, bundler, and the native extensions (see also #12), so here's a pure Ruby version.

/cc @jcoglan

@kbarrette
Copy link
Contributor Author

Build appears to be failing because it's trying to run the no-longer-present compile rake task.

@kbarrette kbarrette force-pushed the pure_ruby_masking branch 2 times, most recently from e355d57 to cf56ea0 Compare November 4, 2016 19:17
@jcoglan
Copy link
Collaborator

jcoglan commented Nov 6, 2016

I know that native extensions can pose a problem on Windows. However, I would rather we solve that problem by fixing the native extensions, than by removing them. The native extension in this gem exists because it provides a significant performance improvement over implementing the same logic in Ruby, and I'm unwilling to get rid of it until we've exhausted every possibility for making the native code work with Windows.

@kbarrette
Copy link
Contributor Author

@jcoglan I thought that might be the reason for the native extensions. Can you show me how to benchmark the various masking implementations?

Also, how do you feel about pluggable masking? I'd really love to have a pure-Ruby approach here, and I'd like to avoid having to switch to another gem or fork this one.

@kbarrette
Copy link
Contributor Author

Updated with better-performing pure Ruby implementation.

@jcoglan
Copy link
Collaborator

jcoglan commented Nov 12, 2016

It's a long time since I did the work that introduced the native extensions and I've forgotten what tooling I used at the time, and couldn't give you numbers for current ruby versions. However there are various WS benchmarking tools on github.

@jcoglan
Copy link
Collaborator

jcoglan commented Nov 12, 2016

I'm resistant to going down this path for a couple of reasons.

First, increasing the number of implementations we need to maintain increases the build complexity and the chances for inconsistencies. In future, I would like more of this library to be written in C, because it's very well suited to the type of processing required here. Ideally I would like to drop the Java version, and eventually work on JRuby will let us use the C extension there too. Having to maintain multiple versions of each bit of functionality increases my workload.

Second, a pluggable interface is unlikely to work. The set of functionality covered by C code is likely to change over time and it will be hard to define a stable interface where other implementations could be plugged in. Plugins make plenty of sense for composing functionality -- see the extensions support we already have -- but not for swapping out random bits of performance hacks.

In any case, non of this will solve the problem of a gem with C code in it refusing to compile and install on Windows. Do you know of a fallback mechanism that would allow the gem install even if the C code refuses to compile?

@jcoglan
Copy link
Collaborator

jcoglan commented Nov 12, 2016

As a middle ground, I would accept a solution that gets the gem working on your system, that does not delete all the native code from the gem.

@kbarrette kbarrette force-pushed the pure_ruby_masking branch 2 times, most recently from 5c6c5d6 to 4d65428 Compare November 17, 2016 22:31
@kbarrette
Copy link
Contributor Author

@jcoglan added an additional fallback as per your "middle ground" suggestion

@jcoglan
Copy link
Collaborator

jcoglan commented Dec 3, 2016

Just to clarify, what's the experience like at installation time? I was never clear whether your issues stopped the gem from installing, or whether it installed fine but failed to load the native code.

@kbarrette
Copy link
Contributor Author

@jcoglan the installation went fine, but there was a runtime failure, basically exactly like #12

I've been running a forked version of this gem containing my pure-Ruby code with good success, but I'd love to get this fallback version merged and be able to use the main line gem.

@jcoglan
Copy link
Collaborator

jcoglan commented Dec 6, 2016

Does your code work if you remove the clone? The existing masking code doesn't copy the string, it mutates it in place.

@kbarrette
Copy link
Contributor Author

It should work fine without the clone - updated.

@kbarrette
Copy link
Contributor Author

@jcoglan also, Travis now thinks this is OK (not sure why it failed the previous build)

@jcoglan jcoglan merged commit 4fa9e19 into faye:master Dec 27, 2016
@jcoglan
Copy link
Collaborator

jcoglan commented Dec 27, 2016

Thanks a lot for fixing this. I'm now looking at doing a completely native parser, and that will probably result in there being a wholesale switch between a native parser and a pure ruby one, depending on if I can get the interfaces right.

@kbarrette
Copy link
Contributor Author

@jcoglan thanks for merging! Hopefully you can release a new version soon and I can ditch my forked version of this gem.

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

Successfully merging this pull request may close these issues.

2 participants