Repository navigation
fix(android): convert values assigned to primitive array elements like the v8 runtime - #133
Open
ammarahm-ed wants to merge 2 commits into
Open
ammarahm-ed wants to merge 2 commits into
ammarahm-ed wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Assigning a value that is not already the element type to a primitive Java array element logged this and left the element unwritten:
error: 6isnapi_number_expected.SetArrayElementcalled the strictnapi_get_value_*getters, which reject anything that is not exactly a number (or boolean, or string), and returned. The classic V8 runtime converts the value with the JS rules (NumberValue,Int32Value,BooleanValue,ToString), sofloatArray[0] = undefinedstoresNaN,nullstores0,"3.5"stores3.5. The direct runtime did not log anything, but itsjs_util::get_numberreturned 0 for anything that was not a number, so"3.5"stored0andundefinedstored0instead ofNaN.Both runtimes now convert like the V8 runtime. Every site where a user-supplied value reaches a strict getter is covered:
SetArrayElement): boolean, byte, char, short, int, long, float and double arrays. Along[]given an object still goes throughConvertToJavaLong, as in the V8 runtime.getValueAtIndex/setValueAtIndexindex.float(x),double(x)andlong(x)casts passed to a Java method that takesObject(JsArgToArrayConverter).onerrorhandler return value (Node-API only; the flag was also read uninitialised when the conversion failed).How it avoids slowing the fast path
Each conversion tries the existing strict getter first, so a value of the right type does exactly what it did. Only when that fails does it coerce (
napi_coerce_to_number/_to_bool/_to_string, or the globalNumber/Booleanin the direct runtime). If coercion fails too, the same Node-API error is logged as before and nothing is thrown; any exception the coercion raised (for example from aSymbol) is cleared. The strict-then-coerce helpers are in a newCoercion.hin each tree.Results
Debug build on a Sony SO-01M (Android 11, arm64), V8, both runtimes, the Jasmine suite. The first commit adds five specs to
testArrays.js; the second is the fix.The fast path, from the existing benchmark rows, release build, 5 launches, median, ns per assignment:
int[0] = ndouble[0] = nboolean[0] = bString[0] = sint[0]readThe median over all benchmark rows is +0.6% on Node-API and -0.1% on the direct runtime. The writes that now do one extra range check are 2–4% slower, which is inside this device's noise of about ±5% per row; I would not call it a regression, but I did not measure it below that.
Notes
obj.floatField = x) were not touched: they already check the type and store 0 for anything else, so they never raised this error, though they do not produceNaNlike the V8 runtime would.napi_coerce_to_number,_to_booland_to_stringfrom each engine shim; I have not built or checked the other engines.maindoes not link V8 on Android until fix(android/v8): skip the ES module cleanup the Android build does not link #119 merges, so the builds above carried that one fix underneath this change. It is not part of this PR.