Skip to content

build: enable v8's SipHash for hash seed creation #26367

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

build: enable v8's SipHash for hash seed creation #26367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72a1800
build: enable v8's siphash for hash seed creation
rvagg Mar 1, 2019
a155578
deps,tools: include SipHash in LICENSE
rvagg Mar 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions LICENSE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -774,6 +774,17 @@ The externally maintained libraries used by Node.js are:
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
"""

- SipHash, located at deps/v8/src/third_party/siphash, is licensed as follows:
"""
SipHash reference C implementation

Copyright (c) 2016 Jean-Philippe Aumasson <[email protected]>

To the extent possible under law, the author(s) have dedicated all
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC CC0 is a "no-attribution no-copyright" license, which IMO makes explicitly including it counter productive.

copyright and related and neighboring rights to this software to the public
domain worldwide. This software is distributed without any warranty.
"""

- zlib, located at deps/zlib, is licensed as follows:
"""
zlib.h -- interface of the 'zlib' general purpose compression library
Expand Down
3 changes: 3 additions & 0 deletions common.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@
# Old time default, now explicitly stated.
'v8_use_snapshot': 'true',

# Turn on SipHash for hash seed generation, addresses HashWick
'v8_use_siphash': 'true',

# These are more relevant for V8 internal development.
# Refs: https://github.com/nodejs/node/issues/23122
# Refs: https://github.com/nodejs/node/issues/23167
Expand Down
6 changes: 6 additions & 0 deletions configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,11 @@
dest='without_snapshot',
help=optparse.SUPPRESS_HELP)

parser.add_option('--without-siphash',
action='store_true',
dest='without_siphash',
help=optparse.SUPPRESS_HELP)

parser.add_option('--code-cache-path',
action='store',
dest='code_cache_path',
Expand Down Expand Up @@ -1122,6 +1127,7 @@ def configure_v8(o):
o['variables']['v8_random_seed'] = 0 # Use a random seed for hash tables.
o['variables']['v8_promise_internal_field_count'] = 1 # Add internal field to promises for async hooks.
o['variables']['v8_use_snapshot'] = 'false' if options.without_snapshot else 'true'
o['variables']['v8_use_siphash'] = 'false' if options.without_siphash else 'true'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity: this could be just o['variables']['v8_use_siphash'] = b(options.without_siphash) but it's fine, it's still locally consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the tip, we could do a lot of simplification with that but I'll withhold this time

Copy link
Contributor

@refack refack Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need a ./configure flag?
IIUC this is an obscure enough configuration that doing:

./configure -- -Dv8_use_siphash=false

seems good enough

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be removed in a future version when someone's cleaning up. For now if it ends up causing problems, performance or backward compatibility, or whatever, at least they have an easy out

o['variables']['v8_trace_maps'] = 1 if options.trace_maps else 0
o['variables']['node_use_v8_platform'] = b(not options.without_v8_platform)
o['variables']['node_use_bundled_v8'] = b(not options.without_bundled_v8)
Expand Down
3 changes: 3 additions & 0 deletions deps/v8/gypfiles/features.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,9 @@
['v8_use_snapshot=="true" and v8_use_external_startup_data==1', {
'defines': ['V8_USE_EXTERNAL_STARTUP_DATA',],
}],
['v8_use_siphash=="true"', {
'defines': ['V8_USE_SIPHASH',],
}],
['dcheck_always_on!=0', {
'defines': ['DEBUG',],
}],
Expand Down
3 changes: 3 additions & 0 deletions deps/v8/gypfiles/v8.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@
'v8_enable_verify_predictable=<(v8_enable_verify_predictable)',
'v8_target_cpu=<(v8_target_arch)',
'v8_use_snapshot=<(v8_use_snapshot)',
'v8_use_siphash=<(v8_use_siphash)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh. We've been missing this with some of the other V8 new features...
/me writes himself a TODO note

]
},
'conditions': [
Expand Down Expand Up @@ -1528,6 +1529,8 @@
'../src/string-stream.h',
'../src/strtod.cc',
'../src/strtod.h',
'../src/third_party/siphash/halfsiphash.cc',
'../src/third_party/siphash/halfsiphash.h',
'../src/third_party/utf8-decoder/utf8-decoder.h',
'../src/torque-assembler.h',
'../src/tracing/trace-event.cc',
Expand Down
1 change: 1 addition & 0 deletions node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
'variables': {
'v8_use_snapshot%': 'false',
'v8_use_siphash%': 'true',
'v8_trace_maps%': 0,
'node_use_dtrace%': 'false',
'node_use_etw%': 'false',
Expand Down
2 changes: 2 additions & 0 deletions tools/license-builder.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ addlicense "OpenSSL" "deps/openssl" \
addlicense "Punycode.js" "lib/punycode.js" \
"$(curl -sL https://raw.githubusercontent.com/bestiejs/punycode.js/master/LICENSE-MIT.txt)"
addlicense "V8" "deps/v8" "$(cat ${rootdir}/deps/v8/LICENSE)"
addlicense "SipHash" "deps/v8/src/third_party/siphash" \
"$(sed -e '/You should have received a copy of the CC0/,$d' -e 's/^\/\* *//' -e 's/^ \* *//' deps/v8/src/third_party/siphash/halfsiphash.cc)"
addlicense "zlib" "deps/zlib" \
"$(sed -e '/The data format used by the zlib library/,$d' -e 's/^\/\* *//' -e 's/^ *//' ${rootdir}/deps/zlib/zlib.h)"

Expand Down