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

test: support odd value for kStringMaxLength #14476

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jul 25, 2017

V8 6.2 will support a larger maximum string length on 64-bit platforms.
Update the test to take into account its odd value ((1 << 30) - 1 - 24).

Refs: v8/v8@e8c9649
Example failing test on canary: https://ci.nodejs.org/job/node-test-commit-linux/11406/nodes=centos7-64/console

@nodejs/v8

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer, V8

V8 6.2 will support a larger maximum string length on 64-bit platforms.
Update the test to take into account its odd value ((1 << 30) - 1 - 24).

Refs: v8/v8@e8c9649
@targos targos added buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency. labels Jul 25, 2017
@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Jul 25, 2017
@jasnell
Copy link
Member

jasnell commented Jul 27, 2017

@trevnorris
Copy link
Contributor

While I'm looking at this, and in hopes of removing unnecessary tests, this test uses toString('utf16le') and test-stringbytes-external-at-max.js uses toString('latin1'). Both of these will copy the data and turn them an externalized strings. Because they are externalized strings, do they suffer the same limitations as strings that are copied into the heap that need to be tested?

Also, I'm worried that this test now requires allocating 2GB memory. Could we possibly move this test to only run on test-ci, or some such? I've had these tests hang for a while because, while there is enough free memory reported, the kernel is busy moving things into swap because there isn't enough contiguous memory to make the allocation.

@targos
Copy link
Member Author

targos commented Aug 3, 2017

New CI: https://ci.nodejs.org/job/node-test-pull-request/9460/

@trevnorris do you agree to land this and move your concern to a new issue? This test will not allocate 2GB memory until we upgrade to V8 6.2 (in October).

@jasnell
Copy link
Member

jasnell commented Aug 3, 2017

One failure is CI is unrelated to this PR

targos added a commit to targos/node that referenced this pull request Aug 4, 2017
V8 6.2 will support a larger maximum string length on 64-bit platforms.
Update the test to take into account its odd value ((1 << 30) - 1 - 24).

Refs: v8/v8@e8c9649
PR-URL: nodejs#14476
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@targos
Copy link
Member Author

targos commented Aug 4, 2017

Landed in 753b68f

@targos targos closed this Aug 4, 2017
@targos targos deleted the fix-string-test-canary branch August 4, 2017 09:18
@trevnorris
Copy link
Contributor

@targos As long as we remember to change the test in October that's cool.

@gibfahn
Copy link
Member

gibfahn commented Aug 5, 2017

@trevnorris @targos we could add this concern (as a new issue) to the 9.0.0 milestone, as that's also happening in October. IDK if that's a good idea though.

addaleax pushed a commit that referenced this pull request Aug 7, 2017
V8 6.2 will support a larger maximum string length on 64-bit platforms.
Update the test to take into account its odd value ((1 << 30) - 1 - 24).

Refs: v8/v8@e8c9649
PR-URL: #14476
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@targos targos mentioned this pull request Oct 18, 2017
@saqirltu
Copy link

New CI: https://ci.nodejs.org/job/node-test-pull-request/9460/

@trevnorris do you agree to land this and move your concern to a new issue? This test will not allocate 2GB memory until we upgrade to V8 6.2 (in October).

Hi, is it really possible to do this? Hereby from 2019 still not being able to try it yet. Could you please check out my thread? Thanks!
https://npm.community/t/cannot-create-a-string-longer-than-0x3fffffe7-characters/10738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addons Issues and PRs related to native addons. buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.