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

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

Closed
wants to merge 7 commits into
base: maint
from

Conversation

Projects
None yet
3 participants
@potatosalad
Copy link
Contributor

potatosalad commented Jun 10, 2015

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

This comment has been minimized.

Copy link

OTP-Maintainer commented Jun 11, 2015

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


I am a script, I am not human


@potatosalad potatosalad force-pushed the potatosalad:binary_split_bif branch from 2f79777 to 7ff7665 Jun 25, 2015

@OTP-Maintainer

This comment has been minimized.

Copy link

OTP-Maintainer commented Jun 30, 2015

./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

This comment has been minimized.

Copy link
Contributor Author

potatosalad commented Jun 30, 2015

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

This comment has been minimized.

Copy link
Contributor

sverker commented Aug 26, 2015

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

This comment has been minimized.

Copy link
Contributor Author

potatosalad commented Aug 26, 2015

@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

This comment has been minimized.

Copy link
Contributor

sverker commented Aug 26, 2015

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

This comment has been minimized.

Copy link

OTP-Maintainer commented Aug 27, 2015

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 potatosalad referenced this pull request Sep 7, 2015

Merged

Erlang implementation #10

@potatosalad potatosalad force-pushed the potatosalad:binary_split_bif branch from 02cf385 to 87b90a2 Sep 11, 2015

@potatosalad

This comment has been minimized.

Copy link
Contributor Author

potatosalad commented Sep 11, 2015

@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

This comment has been minimized.

Copy link
Contributor

sverker commented Sep 15, 2015

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).

erts: Minor refactor for binary find BIF backend
* Use NULL instead of THE_NON_VALUE for non-Eterm variable.
* Add BinaryFindState_bignum struct to avoid unnecessary type casting.
@potatosalad

This comment has been minimized.

Copy link
Contributor Author

potatosalad commented Sep 18, 2015

@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

This comment has been minimized.

Copy link
Contributor

sverker commented Sep 18, 2015

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

@potatosalad

This comment has been minimized.

Copy link
Contributor Author

potatosalad commented Sep 18, 2015

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

@OTP-Maintainer

This comment has been minimized.

Copy link

OTP-Maintainer commented Sep 24, 2015

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


I am a script, I am not human


@sverker

This comment has been minimized.

Copy link
Contributor

sverker commented Nov 5, 2015

Finally merged to master in 1670e9d.

I also joined some of the commits.

@sverker sverker closed this Nov 5, 2015

@potatosalad

This comment has been minimized.

Copy link
Contributor Author

potatosalad commented Nov 5, 2015

@sverker Awesome, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.