stdlib: Add BIF binary:split/2 and binary:split/3

[potatosalad]

While working on an Erlang implementation of the dimroc/etl-language-comparison#10 project, @josevalim pointed out that one of the performance issues was the pure Erlang implementation of binary:split/2 and binary:split/3: [erlang-questions] Trying to understand the performance impact of binary:split/3.

Rough estimates on my machine show a 6x speed up for binary:split/2 and 4x speed up binary:split/3 between the pure Erlang and native BIF implementation.

The implementation itself uses the same Boyer Moore and Aho Corasick algorithms used in binary:match/2,3 and binary:matches/2,3.

Update 2015-06-25: Rebased for OTP-18.0 and added support for the trim_all option added in #517. Rough estimates show a 6-10x speed up for binary:split/3 when using the trim_all option.

[OTP-Maintainer]

Patch has passed first testings and has been assigned to be reviewed

---

I am a script, I am not human

---

[OTP-Maintainer]

./otp_build all -a failed:

c/com/ericsson/otp/erlang'
 JAVAC  /ldisk/jenkins/jobs/pullrequests/workspace/otp/lib/jinterface/priv/com/ericsson/otp/erlang/OtpAuthException.class
OtpAuthException.java:29: error while writing com.ericsson.otp.erlang.OtpAuthException: could not create parent directories
public class OtpAuthException extends OtpException {
       ^
1 error
make[4]: *** [/ldisk/jenkins/jobs/pullrequests/workspace/otp/lib/jinterface/priv/com/ericsson/otp/erlang/OtpAuthException.class] Error 1
make[4]: Leaving directory `/ldisk/jenkins/jobs/pullrequests/workspace/otp/lib/jinterface/java_src/com/ericsson/otp/erlang'
make[3]: *** [opt] Error 2
make[3]: Leaving directory `/ldisk/jenkins/jobs/pullrequests/workspace/otp/lib/jinterface/java_src'
make[2]: *** [opt] Error 2
make[2]: Leaving directory `/ldisk/jenkins/jobs/pullrequests/workspace/otp/lib/jinterface'
make[1]: *** [opt] Error 2
make[1]: Leaving directory `/ldisk/jenkins/jobs/pullrequests/workspace/otp/lib'
make: *** [tertiary_bootstrap_build] Error 2

---

I am a script, I am not human

---

[potatosalad]

I'm unable to reproduce the error reported by @OTP-Maintainer when building from a clean checkout on OS X and SmartOS. I'm assuming it was a resource limitation error while running the automated jenkins build.

[sverker]

The PR with its speedup looks good, but we don't like all the code duplication.

In the spirit to inhibit code bloating the beam, I think some time should be spent on refactoring. To lead the way I have done one refactoring of the "backend" for split
that removed a net of 350 lines.

https://github.com/sverker/otp/commits/sverk/binary_split_bif

I see two approaches for further code reduce:

1. Make a common interface for the BM and AC search algorithms that would avoid duplicating the client code.
2. Combine match(es) and split into one match-engine with different frontends (the bif callers) and
   different backends (the return value construction).

[potatosalad]

@sverker Thank you for refactoring the bloated code. I admit that I had more code duplicated than was necessary while I was debugging each of the 4 branches a split could follow (global/non-global and BM/AC).

When I was implementing this BIF, I wasn't sure whether touching all of the match/matches implementation or trying to keep this BIF isolated was better code/contribution etiquette, so I opted for the latter. I do agree, however, that it would make more sense to deduplicate the very similar match/matches/split underlying implementations.

I see two approaches for further code reduce:

1. Make a common interface for the BM and AC search algorithms that would avoid duplicating the client code.

This option makes the most sense to me, in part because of my lack of knowledge explained below.

2. Combine match(es) and split into one match-engine with different frontends (the bif callers) and
different backends (the return value construction).

I don't entirely understand how everything would function code-organization-wise, especially the BIF traps. Would there just be a single trap for match/matches/split? I'm probably not envisioning things here as you intend, but it will probably click and make sense for me later 😄

For your sverk/binary_split_bif branch, would you like me to merge that into my branch so your changes are present in this pull request, or do you want to wait until any refactoring has been finalized?

[sverker]

Do a fast-forward merge of my commits into your branch.

I haven't mapped out all the details of #2, but it would probably be one common trap
with the argument telling which backend to use.
I suggest you do #1 and then we'll see how much to gain with going further with #2.

[OTP-Maintainer]

Fetching pull requests from github failed:

Host key verification failed.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.

---

I am a script, I am not human

---

[potatosalad]

@sverker I just rebased everything onto maint with with my refactor changes included. I'm not sure why I didn't initially understand about your suggestion for the unified trap, but I believe I have what you were suggesting implemented now.

Here is the list of changes in the latest commit:

1. Remove binary_match_trap, binary_matches_trap, and binary_split_trap and replace with unified binary_find_trap
2. Remove do_binary_match, do_binary_matches, and do_binary_split and replace with unified do_binary_find
3. Added the functions do_match_not_found_result, do_match_single_result, do_match_global_result, and do_split_not_found_result which go along with the functions introduced by @sverker in efdc3f1
4. Added a struct BinaryFindState which holds the type, flags, scope, and pointers to the functions from 3

The net change in lines of code from efdc3f1 for erts/emulator/beam/erl_bif_binary.c was 305 lines removed.

All of the binary_module_SUITE.erl tests are also passing for me. Please let me know what you think and whether any further refactoring should be done.

[sverker]

Looks good.

Nittpick #1: Third argument to do_binary_find should be NULL instead of THE_NON_VALUE. You get
a warning for this when building for debug target, as THE_NON_VALUE is not 0 then.

Nittpick #2: It would nice if you made a struct something like this:

struct BinaryFindState_bignum {
    Eterm bignum_hdr;
    BinaryFindState bfs;
    union {
        BMFindFirstState bmffs;
        BMFindAllState   bmfas;
        ACFindFirstState acffs;
        ACFindAllState   acfas;
    }u;
};

and then use that as type for state_ptr. I think that will make the code nicer when returning
DO_BIN_MATCH_RESTART without a lot of sizeof and type cast.
(Feel free to change my naming if you want).

[potatosalad]

@sverker I just committed changes for nitpicks 1 and 2, hopefully that accomplishes what you had in mind. Let me know if you notice anything else that needs changing.

[sverker]

Oh, you didn't read my mind ;-)
I meant something like this: sverker@d3b250d

[potatosalad]

@sverker That makes sense and looks much cleaner now. Your commit has been merged into this PR.

[OTP-Maintainer]

Patch has passed first testings and has been assigned to be reviewed

---

I am a script, I am not human

---

[sverker]

Finally merged to master in 1670e9d.

I also joined some of the commits.

[potatosalad]

@sverker Awesome, thank you!