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

Do not allocate good and bad shifts for single byte lookups #1932

Merged
merged 1 commit into from Aug 23, 2018

Conversation

Projects
None yet
3 participants
@josevalim
Copy link
Contributor

josevalim commented Aug 16, 2018

The single byte lookups always rely on memchr and
never really use the good and bad shifts arrays.

My benchmarks show this makes compile_pattern roughly twice
faster for single bytes. It also uses 256 bytes less memory.

Another alternative approach would be to introduce a complete separate
structure that does the single byte implementation but I personally find the
current approach simpler.

Follow up to #1803, fully closes https://bugs.erlang.org/browse/ERL-374.

Do not allocate good and bad shifts for single byte lookups
The single byte lookups always rely on `memchr` and
never really use the good and bad shifts arrays.

#define AC_SIZE(StrLens) /* StrLens: sum of all searchstring lengths */ \
((MYALIGN(sizeof(ACNode)) * \
((StrLens)+1)) + /* The actual nodes (including rootnode) */ \
MYALIGN(sizeof(ACTrie))) /* Structure */

/*

This comment has been minimized.

@josevalim

josevalim Aug 16, 2018

Contributor

The next chunk of changes is literally me moving code around. No actual changes done here.

@@ -390,7 +486,14 @@ static BMData *create_bmdata(MyAllocator *my, byte *x, Uint len,
bmd->x = my_alloc(my,len);
sys_memcpy(bmd->x,x,len);
bmd->len = len;
bmd->goodshift = my_alloc(my,sizeof(Uint) * len);

if(len > 1) {

This comment has been minimized.

@josevalim

josevalim Aug 16, 2018

Contributor

For the case len is 1, is it ok to not do anything with bmd->goodshift and bmd->badshift? Or should we set them to NULL or something similar?

This comment has been minimized.

@garazdawi

garazdawi Aug 20, 2018

Contributor

should be ok to leave it as is. I guess valgrind will tell us if it is not :)


#define BM_SIZE_MULTI(StrLen) /* StrLen: length of searchstring */ \
((MYALIGN(sizeof(Uint) * (StrLen))) + /* goodshift array */ \
(MYALIGN(sizeof(Uint) * ALPHABET_SIZE)) + /* badshift array */ \

This comment has been minimized.

@josevalim

josevalim Aug 16, 2018

Contributor

Note the previous version used sizeof(Sint) but the code actually uses Uint. So I changed this to Uint for consistency.

@garazdawi garazdawi self-assigned this Aug 20, 2018

@garazdawi
Copy link
Contributor

garazdawi left a comment

The code looks good.

Just so that I'm getting this change: The benefit is that we no longer have to allocate the badshift dictionary, i.e. 256 words of memory?

@@ -390,7 +486,14 @@ static BMData *create_bmdata(MyAllocator *my, byte *x, Uint len,
bmd->x = my_alloc(my,len);
sys_memcpy(bmd->x,x,len);
bmd->len = len;
bmd->goodshift = my_alloc(my,sizeof(Uint) * len);

if(len > 1) {

This comment has been minimized.

@garazdawi

garazdawi Aug 20, 2018

Contributor

should be ok to leave it as is. I guess valgrind will tell us if it is not :)

MYALIGN(StrLen) + /* searchstring saved */ \
(MYALIGN(sizeof(BMData)))) /* Structure */
#define BM_SIZE_SINGLE() /* Single byte search string */ \
(MYALIGN(1) + /* searchstring saved */ \

This comment has been minimized.

@garazdawi

garazdawi Aug 20, 2018

Contributor

Should this be sizeof(byte)?

This comment has been minimized.

@josevalim

josevalim Aug 20, 2018

Contributor

In the snippet above, this is equivalent to MYALIGN(StrLen). In our case StrLen is 1. So I thought about passing 1. So maybe above should be MYALIGN(StrLen * byte()) too?

This comment has been minimized.

@garazdawi

garazdawi Aug 20, 2018

Contributor

Strictly speaking yes, but I think we can leave it as is. It looked strange when I first looked at it, but having looked closer it is fine.

@josevalim

This comment has been minimized.

Copy link
Contributor

josevalim commented Aug 20, 2018

Just so that I'm getting this change: The benefit is that we no longer have to allocate the badshift dictionary, i.e. 256 words of memory?

Yup, we don't need to allocate nor process it.

@garazdawi garazdawi merged commit a1a7222 into erlang:master Aug 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@josevalim josevalim deleted the josevalim:jv-sb-bm branch Aug 23, 2018

@josevalim

This comment has been minimized.

Copy link
Contributor

josevalim commented Aug 23, 2018

Thanks @garazdawi, I believe https://bugs.erlang.org/browse/ERL-374 can be closed too.

@garazdawi

This comment has been minimized.

Copy link
Contributor

garazdawi commented Aug 23, 2018

Done, thanks!

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