Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Timob 12930 - Reduce redundant checks on env #3933

Merged
merged 5 commits into from Mar 23, 2013

Conversation

Projects
None yet
4 participants
Contributor

BlainHamon commented Mar 5, 2013

This is a very safe optimization. Test will be poking with KS to make sure we don't dereference null in passing data from Android to V8. ( Question: Should this be wrapped into a larger pull request? )

@ghost ghost assigned ayeung Mar 19, 2013

Contributor

joshthecoder commented Mar 21, 2013

We should verify old modules remain compatible unless we are okay breaking ABI next release.
It may not be safe adding an overloaded method here.

Contributor

joshthecoder commented Mar 21, 2013

Also if ABI is breaking we might as well just remove the old signatures (that take no env).
If modules need to be re-built they will use the newer API since the glue will be re-generated.

Contributor

ayeung commented Mar 21, 2013

So with this changes, new modules will most likely be forward compatible but not backwards compatible. It also looks like you missed some signatures in ProxyBinding.fm. Will test again once those changes have been made.

Contributor

BlainHamon commented Mar 21, 2013

Modules made for 3.1.0 with this wouldn't be compatible pre-3.1.0, but when you make a module, the manifest already states 3.1.0 as the min SDK. Older modules shouldn't have a problem with 3.1.0 since the old methods with the old signatures remain.

@mstepanov mstepanov commented on the diff Mar 22, 2013

android/runtime/v8/src/native/TypeConverter.cpp
@@ -305,13 +353,23 @@ jshortArray TypeConverter::jsArrayToJavaShortArray(v8::Handle<v8::Array> jsArray
return javaDoubleArrayToJsNumberArray((jdoubleArray) javaShortArray);
}
+v8::Handle<v8::Array> TypeConverter::javaArrayToJsArray(JNIEnv *env, jshortArray javaShortArray)
+{
+ return javaDoubleArrayToJsNumberArray(env, (jdoubleArray) javaShortArray);
@mstepanov

mstepanov Mar 22, 2013

Contributor

why cast short[] to double[] ?

@BlainHamon

BlainHamon Mar 22, 2013

Contributor

You know, I'm not quite sure. It's a copy-paste of the entry above which doesn't have env. Blame says it's from commit 221628e

@mstepanov

mstepanov Mar 22, 2013

Contributor

That commit has only formatting changes.

@BlainHamon

BlainHamon Mar 22, 2013

Contributor

Answer: Because the compiler says so. Removing the cast gives:

[exec] src/native/TypeConverter.cpp:353:54: error: no matching function for call to 'titanium::TypeConverter::javaDoubleArrayToJsNumberArray(_jshortArray*&)'

@mstepanov

mstepanov Mar 22, 2013

Contributor

because it should use non-existing v8::Handlev8::Array javaShortArrayToJsNumberArray(jshortArray javaShortArray);

@BlainHamon

BlainHamon Mar 22, 2013

Contributor

Agreed. And they should be optimized. In a new Jira.

@mstepanov

mstepanov Mar 22, 2013

Contributor

Created TIMOB-13170 to address it.

TIMOB-12930 Update the tempate to pass along env; add inline function…
…s to handle env-less primative conversions as a result.

@mstepanov mstepanov and 1 other commented on an outdated diff Mar 22, 2013

android/runtime/v8/src/native/TypeConverter.cpp
@@ -436,19 +518,28 @@ jdoubleArray TypeConverter::jsArrayToJavaDoubleArray(v8::Handle<v8::Array> jsArr
return javaDoubleArrayToJsNumberArray((jdoubleArray) javaDoubleArray);
}
+v8::Handle<v8::Array> TypeConverter::javaArrayToJsArray(JNIEnv *env, jdoubleArray javaDoubleArray)
+{
+ return javaDoubleArrayToJsNumberArray(env, (jdoubleArray) javaDoubleArray);
@mstepanov

mstepanov Mar 22, 2013

Contributor

remove cast

@BlainHamon

BlainHamon Mar 22, 2013

Contributor

Done.

Contributor

mstepanov commented Mar 22, 2013

@BlainHamon address comments

Contributor

BlainHamon commented Mar 22, 2013

Ready for rererererereview.

Contributor

mstepanov commented Mar 22, 2013

Code reviewed. APPROVED

Contributor

mstepanov commented Mar 22, 2013

@ayeung @joshthecoder second CR on you

Contributor

ayeung commented Mar 23, 2013

Code reviewed and functionally tested. Ran test against modules, and KS. Request Accepted.

ayeung pushed a commit that referenced this pull request Mar 23, 2013

Merge pull request #3933 from BlainHamon/timob-12930
Timob 12930 - Reduce redundant checks on env

@ayeung ayeung merged commit 97a083b into appcelerator:master Mar 23, 2013

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