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

API to make a PropertyId directly from a JavaScript string #3623

Closed
fatcerberus opened this Issue Aug 31, 2017 · 12 comments

Comments

Projects
None yet
5 participants
@fatcerberus
Contributor

fatcerberus commented Aug 31, 2017

Currently to create a property ID from a string passed in from JS, I have to do:

JsCopyString(key_ref, key_string, ...);
JsCreatePropertyId(key_string, key_length, &prop_id);

JsGetPropertyIdFromSymbol() takes a JsValueRef as argument, but that only works for symbols.

@liminzhu

This comment has been minimized.

Show comment
Hide comment
@liminzhu

liminzhu Sep 1, 2017

Member

Thanks for the feedback @fatcerberus . I am curious why you're creating propertyId from JS strings? Usually hosts know exactly what property they look for and create propertyId from a native string (e.g. when hosts want to call built-ins which are properties of global object).

Member

liminzhu commented Sep 1, 2017

Thanks for the feedback @fatcerberus . I am curious why you're creating propertyId from JS strings? Usually hosts know exactly what property they look for and create propertyId from a native string (e.g. when hosts want to call built-ins which are properties of global object).

@fatcerberus

This comment has been minimized.

Show comment
Hide comment
@fatcerberus

fatcerberus Sep 1, 2017

Contributor

My use case is admittedly off the beaten path. My application, miniSphere, a JavaScript game engine, was originally based on Duktape. Duktape has a stack-based API like Lua, and in order to avoid rewriting The entire engine when migrating to Chakra, I recreated the Duktape API.

In particular, my jsal_get_prop() function involves pushing a string onto the stack specifying the property to read. Pushing something onto the stack means creating a JsValueRef from it, so by the time the property read happens, all I have access to is the value ref:
https://github.com/fatcerberus/minisphere/blob/chakra-js/src/shared/jsal.c#L557-L573

Contributor

fatcerberus commented Sep 1, 2017

My use case is admittedly off the beaten path. My application, miniSphere, a JavaScript game engine, was originally based on Duktape. Duktape has a stack-based API like Lua, and in order to avoid rewriting The entire engine when migrating to Chakra, I recreated the Duktape API.

In particular, my jsal_get_prop() function involves pushing a string onto the stack specifying the property to read. Pushing something onto the stack means creating a JsValueRef from it, so by the time the property read happens, all I have access to is the value ref:
https://github.com/fatcerberus/minisphere/blob/chakra-js/src/shared/jsal.c#L557-L573

@liminzhu

This comment has been minimized.

Show comment
Hide comment
@liminzhu

liminzhu Sep 1, 2017

Member

Thanks for sharing @fatcerberus . I think this is an API we could consider. Another use case can be when someone wants to iterate an object's properties natively and calls JsGetOwnPropertyNames, which returns a JsValueRef that contains an array of js strings. However, as you pointed out, there's a fairly easy helper you can write to achieve the same goal, so I doubt this is something we'd immediately do unless there's more demand and a potential perf gain. Does that sound fair to you? And of course, PR's welcome especially if there's a perf gain over the helper :).

Member

liminzhu commented Sep 1, 2017

Thanks for sharing @fatcerberus . I think this is an API we could consider. Another use case can be when someone wants to iterate an object's properties natively and calls JsGetOwnPropertyNames, which returns a JsValueRef that contains an array of js strings. However, as you pointed out, there's a fairly easy helper you can write to achieve the same goal, so I doubt this is something we'd immediately do unless there's more demand and a potential perf gain. Does that sound fair to you? And of course, PR's welcome especially if there's a perf gain over the helper :).

@fatcerberus

This comment has been minimized.

Show comment
Hide comment
@fatcerberus

fatcerberus Sep 2, 2017

Contributor

Fair enough. The main reason using the helper bothered me is that I'm copying a string seemingly unnecessarily when the JS engine could easily reuse the one it already has in its string table. Admittedly I don't know what really goes on under the hood when creating a PropertyId though. :)

In reality though, we're talking about very short strings on average so the overhead of copying a string isn't a big deal most likely.

Contributor

fatcerberus commented Sep 2, 2017

Fair enough. The main reason using the helper bothered me is that I'm copying a string seemingly unnecessarily when the JS engine could easily reuse the one it already has in its string table. Admittedly I don't know what really goes on under the hood when creating a PropertyId though. :)

In reality though, we're talking about very short strings on average so the overhead of copying a string isn't a big deal most likely.

@liminzhu

This comment has been minimized.

Show comment
Hide comment
@liminzhu

liminzhu Sep 2, 2017

Member

Yep this is essentially what I'm thinking when I say perf gain. If there's a scenario where the behavior you mentioned happens too often and/or simply costs too much, then it adds more weight to make a change now.

Member

liminzhu commented Sep 2, 2017

Yep this is essentially what I'm thinking when I say perf gain. If there's a scenario where the behavior you mentioned happens too often and/or simply costs too much, then it adds more weight to make a change now.

@fatcerberus

This comment has been minimized.

Show comment
Hide comment
@fatcerberus

fatcerberus Sep 14, 2017

Contributor

@liminzhu Regarding that perf gain...

We noticed a huge performance regression in miniSphere's API across the board after switching from Duktape to ChakraCore. Many calls became 2-3x slower for no obvious reason. With some profiling, we tracked down the cause of the regression to excessive use of JsCopyString:
http://forums.spheredev.org/index.php/topic,1215.msg10552.html#msg10552

After applying the patch below, performance for us improved 20-25%:
fatcerberus/minisphere@ed565a2

Trouble is, that involves calling JsStringToPointer which is deprecated outside of Windows. Which brings me back to the proposal here... 😉

Contributor

fatcerberus commented Sep 14, 2017

@liminzhu Regarding that perf gain...

We noticed a huge performance regression in miniSphere's API across the board after switching from Duktape to ChakraCore. Many calls became 2-3x slower for no obvious reason. With some profiling, we tracked down the cause of the regression to excessive use of JsCopyString:
http://forums.spheredev.org/index.php/topic,1215.msg10552.html#msg10552

After applying the patch below, performance for us improved 20-25%:
fatcerberus/minisphere@ed565a2

Trouble is, that involves calling JsStringToPointer which is deprecated outside of Windows. Which brings me back to the proposal here... 😉

@rhuanjl

This comment has been minimized.

Show comment
Hide comment
@rhuanjl

rhuanjl Sep 14, 2017

Contributor

Thought I'd comment on this briefly as I did a lot of the aforementioned profiling.

To run the helper shown above you have to do:

  1. Find the length of the string: call JSCopyString with no buffer to get the length back (this does one UTf16->UTF8 conversion
  2. Allocate a buffer
  3. Call JSCopy String againg (second conversion)
  4. Now call JsCreatePropertyId passing it the string -> this converts UTF8 back to UTF16

Considering that ultimately all you're trying to do is find the value of a string already held in UTF16 format within the engine and pass it to JsGetPropertyIdFromName the above conversions and memory allocations seem very excessive.

I don't know how to submit a pull request - (sorry I'm new to the ways of github and in fact online collaboration in general) - I think the below code would work as this proposed api and from a few tests it takes 10-20% of the execution time that the helper takes.

CHAKRA_API JSCreatePropertyIDFromJSString(
            _In_ JsValueRef stringValue,
           _Out_ JsPropertyIdRef *propertyId)
{
  const char16* str = nullptr;
  size_t strLength = 0;
  JsErrorCode errorCode = JsStringToPointer(stringValue, &str, &strLength);
  if (errorCode != JsNoError)
  {
    return errorCode;
  }
  return JsGetPropertyIdFromName(str, propertyId);
}
Contributor

rhuanjl commented Sep 14, 2017

Thought I'd comment on this briefly as I did a lot of the aforementioned profiling.

To run the helper shown above you have to do:

  1. Find the length of the string: call JSCopyString with no buffer to get the length back (this does one UTf16->UTF8 conversion
  2. Allocate a buffer
  3. Call JSCopy String againg (second conversion)
  4. Now call JsCreatePropertyId passing it the string -> this converts UTF8 back to UTF16

Considering that ultimately all you're trying to do is find the value of a string already held in UTF16 format within the engine and pass it to JsGetPropertyIdFromName the above conversions and memory allocations seem very excessive.

I don't know how to submit a pull request - (sorry I'm new to the ways of github and in fact online collaboration in general) - I think the below code would work as this proposed api and from a few tests it takes 10-20% of the execution time that the helper takes.

CHAKRA_API JSCreatePropertyIDFromJSString(
            _In_ JsValueRef stringValue,
           _Out_ JsPropertyIdRef *propertyId)
{
  const char16* str = nullptr;
  size_t strLength = 0;
  JsErrorCode errorCode = JsStringToPointer(stringValue, &str, &strLength);
  if (errorCode != JsNoError)
  {
    return errorCode;
  }
  return JsGetPropertyIdFromName(str, propertyId);
}
@rhuanjl

This comment has been minimized.

Show comment
Hide comment
@rhuanjl

rhuanjl Sep 14, 2017

Contributor

@liminzhu Further to the above I'd be happy to a) learn how to do a pull request and b) submit one

That is, if this API is wanted. I admit the only usage cases I can think of are
a) miniSphere due to the way its internals are built as Fat Cerberus mentioned above and
b) your suggestion of someone wanting to iterate an objects' own properties with JsGetOwnPropertyNames; if anyone's doing that a lot this API is worth providing for performance reasons.

Contributor

rhuanjl commented Sep 14, 2017

@liminzhu Further to the above I'd be happy to a) learn how to do a pull request and b) submit one

That is, if this API is wanted. I admit the only usage cases I can think of are
a) miniSphere due to the way its internals are built as Fat Cerberus mentioned above and
b) your suggestion of someone wanting to iterate an objects' own properties with JsGetOwnPropertyNames; if anyone's doing that a lot this API is worth providing for performance reasons.

@liminzhu

This comment has been minimized.

Show comment
Hide comment
@liminzhu

liminzhu Sep 18, 2017

Member

Thanks for the input @fatcerberus @rhuanjl !

The perf gap between JsCopyString and JsStringToPointer is concerning to begin with. @obastemur are you aware of this?

Member

liminzhu commented Sep 18, 2017

Thanks for the input @fatcerberus @rhuanjl !

The perf gap between JsCopyString and JsStringToPointer is concerning to begin with. @obastemur are you aware of this?

@obastemur

This comment has been minimized.

Show comment
Hide comment
@obastemur

obastemur Sep 21, 2017

Member

The perf gap between JsCopyString and JsStringToPointer is concerning to begin with. @obastemur are you aware of this?

Nothing surprising here. ChakraCore stores string variables in multibyte format. JsStringToPointer simply returns a reference to whatever CC has in memory. No copy, no re-formatting.. nothing..

Well, JsCopyString does almost everything... re-format, copy...

We've been doing some improvements to Jsrt and more to come. Yet, I wouldn't expect the performance of JsStringToPointer.

/cc @curtisman @digitalinfinity

Member

obastemur commented Sep 21, 2017

The perf gap between JsCopyString and JsStringToPointer is concerning to begin with. @obastemur are you aware of this?

Nothing surprising here. ChakraCore stores string variables in multibyte format. JsStringToPointer simply returns a reference to whatever CC has in memory. No copy, no re-formatting.. nothing..

Well, JsCopyString does almost everything... re-format, copy...

We've been doing some improvements to Jsrt and more to come. Yet, I wouldn't expect the performance of JsStringToPointer.

/cc @curtisman @digitalinfinity

@rhuanjl

This comment has been minimized.

Show comment
Hide comment
@rhuanjl

rhuanjl Sep 22, 2017

Contributor

Does this issue: #3790
replace this one? @obastemur

Contributor

rhuanjl commented Sep 22, 2017

Does this issue: #3790
replace this one? @obastemur

@obastemur

This comment has been minimized.

Show comment
Hide comment
@obastemur

obastemur Sep 24, 2017

Member

@rhuanjl yes. Thanks for tagging.

Member

obastemur commented Sep 24, 2017

@rhuanjl yes. Thanks for tagging.

@obastemur obastemur closed this Sep 24, 2017

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