Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11360114:
- Add functionality to morph a string into an external string (Closed)
|
Created:
8 years, 1 month ago by siva Modified:
8 years, 1 month ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Visibility:
Public. |
Description- Add functionality to morph a string into an external string
- Add Dart API call Dart_MakeExternalString
Committed: https://code.google.com/p/dart/source/detail?r=14978
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 23
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 8
Patch Set 8 : #
Messages
Total messages: 13 (0 generated)
lgtm. http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl.cc File vm/dart_api_impl.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl.cc#newcod... vm/dart_api_impl.cc:1719: RETURN_TYPE_ERROR(isolate, str, String); Is passing in an external string necessarily an error? The original handle could instead be returned.
http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h File include/dart_api.h (right): http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1487: * Converts a String into an ExternalString. From the documentation and a signature it's not obvious that this function morphs existing string into externalize one. And another possibly handy thing: V8 API can refuse to externalize the string. Originally it was necessary on x64 as very short strings cannot be externalized. Later it was abused to tweak externalization policy: oftentimes it's useless to externalize very young strings as we do not know if they go away soon or not. http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1500: intptr_t length, how embedder can find out this length? http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl_test.cc File vm/dart_api_impl_test.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl_test.cc#n... vm/dart_api_impl_test.cc:7269: EXPECT_EQ(expected_length, length); may you add a test that it's the same object? http://codereview.chromium.org/11360114/diff/10001/vm/object.cc File vm/object.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode487 vm/object.cc:487: if (original_size > used_size) { maybe turn that into an assert? http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode496 vm/object.cc:496: tags = RawObject::SizeTag::update(leftover_size, tags); may there be any issues with alignment? maybe add an assert that all sizes are properly aligned if it's indeed the case? http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode10367 vm/object.cc:10367: ASSERT(original_size >= used_size); is it always true? something like zero length string on x64 platform? sorry, I don't know layout of Dart VM objects so it might be a trivial question. http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode10370 vm/object.cc:10370: memmove(array, OneByteString::CharAddr(*this, 0), str_length); that might be very unpleasant thing when one attempts to externalize a long string. Cannot we ask an embedder to provide data?
http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h File include/dart_api.h (right): http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1487: * Converts a String into an ExternalString. On 2012/11/07 10:05:46, Anton Muhin wrote: > From the documentation and a signature it's not obvious that this function > morphs existing string into externalize one. I am assuming converts means it morphs the existing object. I can add a line in the description making that explicit. > > And another possibly handy thing: V8 API can refuse to externalize the string. > Originally it was necessary on x64 as very short strings cannot be externalized. > Later it was abused to tweak externalization policy: oftentimes it's useless to > externalize very young strings as we do not know if they go away soon or not. I am not sure I understand the need to refuse externalization. We don't have the issue that V8 had with regards to short strings as we always align all objects and the external data is just one word long in the object. The embedder should decide whether to externalize or not externalize a string. Once called I would think the string should be externalized. http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1500: intptr_t length, This is the length of array passed in by the embedder. Since it creates the external space it would know the length. It can try and figure an appropriate length to be used by looking at Dart_StringLength and the type of the string (ascii or not). On 2012/11/07 10:05:46, Anton Muhin wrote: > how embedder can find out this length? http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl.cc File vm/dart_api_impl.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl.cc#newcod... vm/dart_api_impl.cc:1719: RETURN_TYPE_ERROR(isolate, str, String); Yes we could. Does that make sense however, it seems like a lot of code to return back the same handle. I am wondering if the embedder code would prefer knowing when this happens. On 2012/11/07 00:38:43, Tom Ball wrote: > Is passing in an external string necessarily an error? The original handle > could instead be returned. http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl_test.cc File vm/dart_api_impl_test.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/dart_api_impl_test.cc#n... vm/dart_api_impl_test.cc:7269: EXPECT_EQ(expected_length, length); On 2012/11/07 10:05:46, Anton Muhin wrote: > may you add a test that it's the same object? Done. http://codereview.chromium.org/11360114/diff/10001/vm/object.cc File vm/object.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode487 vm/object.cc:487: if (original_size > used_size) { I did not want it to be an assert because then the caller has to do the check. This way the caller just calls this method and if there is no left over space nothing is done. On 2012/11/07 10:05:46, Anton Muhin wrote: > maybe turn that into an assert? http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode496 vm/object.cc:496: tags = RawObject::SizeTag::update(leftover_size, tags); Objects are always aligned, we have asserts for this in RawObject::SizeToTagValue(...). On 2012/11/07 10:05:46, Anton Muhin wrote: > may there be any issues with alignment? maybe add an assert that all sizes are > properly aligned if it's indeed the case? http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode10367 vm/object.cc:10367: ASSERT(original_size >= used_size); Yes this is always true as we always align objects and the external data representation is just one word. On 2012/11/07 10:05:46, Anton Muhin wrote: > is it always true? something like zero length string on x64 platform? sorry, I > don't know layout of Dart VM objects so it might be a trivial question. http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode10370 vm/object.cc:10370: memmove(array, OneByteString::CharAddr(*this, 0), str_length); I am not sure I understand your suggestion here. The embedder would have to extract the data out in someway to provide it right. Are you suggesting that we should copy the data using a loop outside the NoGCScope? On 2012/11/07 10:05:46, Anton Muhin wrote: > that might be very unpleasant thing when one attempts to externalize a long > string. Cannot we ask an embedder to provide data?
http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h File include/dart_api.h (right): http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1487: * Converts a String into an ExternalString. On 2012/11/07 21:43:27, siva wrote: > On 2012/11/07 10:05:46, Anton Muhin wrote: > > From the documentation and a signature it's not obvious that this function > > morphs existing string into externalize one. > > I am assuming converts means it morphs the existing object. I can add a line in > the description making that explicit. Up to you, Siva. > > And another possibly handy thing: V8 API can refuse to externalize the string. > > > Originally it was necessary on x64 as very short strings cannot be > externalized. > > Later it was abused to tweak externalization policy: oftentimes it's useless > to > > externalize very young strings as we do not know if they go away soon or not. > > I am not sure I understand the need to refuse externalization. We don't have the > issue that V8 had with regards to short strings as we always align all objects > and > the external data is just one word long in the object. > > The embedder should decide whether to externalize or > not externalize a string. Once called I would think the > string should be externalized. That was an optimisation and it was implemented in VM as it's only VM which can understand if the string is young enough. The simplified version of the code which lead to this optimisation is roughly: for (int i = 0; i < BIG_NUMBER; i++) { String s = '............ $i'; dom_call_with_string_conversion(s); } Originally v8 externalized all those short-lived strings which has its price as externalization is not very cheap. The problem is bindings cannnot understand if JS string is a very young or not. So Vitaly implemented an heuristics which checked how young string is (by looking where in new space it has been allocated---if it's too close to top, it's considered to be too young) and refused to externalize such strings. That's heuristics, not mandatory, but we may want to leave a room for it. Again, up to you to decide. http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1500: intptr_t length, I might be missing something, sorry if it's the case. I meant that I found no embedder's API call to find out exact size of the storage into which internal representation of VM string will fit. Yes, there is _IsAscii, but it's to some extent leaking internal implementation to assume optimal_length = Dart_IsAscii ? Dart_StringLength : 2 * Dart_StringLength. As an example, when 32 bits per code point were dropped, it might have affected calculations. May we add a helper function to find out required length? Or do it in v8 way and allow the embedder to control internal representation of the string? On 2012/11/07 21:43:27, siva wrote: > This is the length of array passed in by the embedder. > Since it creates the external space it would know the length. > > It can try and figure an appropriate length to be used by > looking at Dart_StringLength and the type of the string > (ascii or not). > > > On 2012/11/07 10:05:46, Anton Muhin wrote: > > how embedder can find out this length? > http://codereview.chromium.org/11360114/diff/10001/vm/object.cc File vm/object.cc (right): http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode10367 vm/object.cc:10367: ASSERT(original_size >= used_size); Okay. In v8 zero length string would be just a header and externalized string had no room to install a pointer into (because externalized string requires a header too). Glad to know it's not a restriction in VM. Might be worth adding to a test in case this would change later. On 2012/11/07 21:43:27, siva wrote: > Yes this is always true as we always align objects and > the external data representation is just one word. > > On 2012/11/07 10:05:46, Anton Muhin wrote: > > is it always true? something like zero length string on x64 platform? sorry, > I > > don't know layout of Dart VM objects so it might be a trivial question. > http://codereview.chromium.org/11360114/diff/10001/vm/object.cc#newcode10370 vm/object.cc:10370: memmove(array, OneByteString::CharAddr(*this, 0), str_length); Yes, the embedder has to do that, but he hasn't to do that twice if I got the idea of API correctly. Imagine the case when embedder cannot adopt data passed into Dart_MakeExternalString into his string. By adopt I mean bypass copying. In this case what I can do (only accoutnting for utf16 case): EmbedderString s = EmbedderString::createUninitializedUtf16(len); // Just allocates memory for the string, doesn't fill it Dart_StringToUtf16(s, s.data(), len); // Copying data from VM heap into embedder's string. int16_t* buffer = new int16_t[len]; // or somewhat sketchier, buffer = s.data() Dart_MakeExternalString(s, buffer, length); // OOPS, another copy. That's actually how v8 bindings work: http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/V8StringResou... Now in v8 you can just tell VM, here comes utf16 data and it's length, please, use it. On 2012/11/07 21:43:27, siva wrote: > I am not sure I understand your suggestion here. The > embedder would have to extract the data out in someway > to provide it right. > > Are you suggesting that we should copy the data using a loop > outside the NoGCScope? > > On 2012/11/07 10:05:46, Anton Muhin wrote: > > that might be very unpleasant thing when one attempts to externalize a long > > string. Cannot we ask an embedder to provide data? >
http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h File include/dart_api.h (right): http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1487: * Converts a String into an ExternalString. On 2012/11/08 08:05:29, Anton Muhin wrote: > On 2012/11/07 21:43:27, siva wrote: > > On 2012/11/07 10:05:46, Anton Muhin wrote: > > > From the documentation and a signature it's not obvious that this function > > > morphs existing string into externalize one. > > > > I am assuming converts means it morphs the existing object. I can add a line > in > > the description making that explicit. > > Up to you, Siva. > > > > And another possibly handy thing: V8 API can refuse to externalize the > string. > > > > > Originally it was necessary on x64 as very short strings cannot be > > externalized. > > > Later it was abused to tweak externalization policy: oftentimes it's > useless > > to > > > externalize very young strings as we do not know if they go away soon or > not. > > > > I am not sure I understand the need to refuse externalization. We don't have > the > > issue that V8 had with regards to short strings as we always align all objects > > and > > the external data is just one word long in the object. > > > > The embedder should decide whether to externalize or > > not externalize a string. Once called I would think the > > string should be externalized. > > That was an optimisation and it was implemented in VM as it's only VM which can > understand if the string is young enough. > > The simplified version of the code which lead to this optimisation is roughly: > > for (int i = 0; i < BIG_NUMBER; i++) { > String s = '............ $i'; > dom_call_with_string_conversion(s); > } > > Originally v8 externalized all those short-lived strings which has its price as > externalization is not very cheap. The problem is bindings cannnot understand if > JS string is a very young or not. So Vitaly implemented an heuristics which > checked how young string is (by looking where in new space it has been > allocated---if it's too close to top, it's considered to be too young) and > refused to externalize such strings. > > That's heuristics, not mandatory, but we may want to leave a room for it. > > Again, up to you to decide. Anton, the reason I think this "optimization" paid off in V8 is due to the fact that weak handles were only handled during an old collection. The weak handles for StringImpls should not be participating in grouping, so we can collect them during new collections as well. Although I understand that there is an additional benefit in your example of skipping the weak handle creation in the first place.
http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h File include/dart_api.h (right): http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1487: * Converts a String into an ExternalString. I am not quite sure---if memory serves V8 was always able to collect external strings in minor GC. My, but not 100% sure, recollections are this was due to all this additinal overhead of allocating external resource wrapper + another V8 API call, etc. Again, I don't suggest to implement this immediately, I only want to point out it might be useful to have a room for this optimisation later on. And, no, I am not insisting. On 2012/11/08 08:50:47, Ivan Posva wrote: > On 2012/11/08 08:05:29, Anton Muhin wrote: > > On 2012/11/07 21:43:27, siva wrote: > > > On 2012/11/07 10:05:46, Anton Muhin wrote: > > > > From the documentation and a signature it's not obvious that this function > > > > morphs existing string into externalize one. > > > > > > I am assuming converts means it morphs the existing object. I can add a line > > in > > > the description making that explicit. > > > > Up to you, Siva. > > > > > > And another possibly handy thing: V8 API can refuse to externalize the > > string. > > > > > > > Originally it was necessary on x64 as very short strings cannot be > > > externalized. > > > > Later it was abused to tweak externalization policy: oftentimes it's > > useless > > > to > > > > externalize very young strings as we do not know if they go away soon or > > not. > > > > > > I am not sure I understand the need to refuse externalization. We don't have > > the > > > issue that V8 had with regards to short strings as we always align all > objects > > > and > > > the external data is just one word long in the object. > > > > > > The embedder should decide whether to externalize or > > > not externalize a string. Once called I would think the > > > string should be externalized. > > > > That was an optimisation and it was implemented in VM as it's only VM which > can > > understand if the string is young enough. > > > > The simplified version of the code which lead to this optimisation is roughly: > > > > for (int i = 0; i < BIG_NUMBER; i++) { > > String s = '............ $i'; > > dom_call_with_string_conversion(s); > > } > > > > Originally v8 externalized all those short-lived strings which has its price > as > > externalization is not very cheap. The problem is bindings cannnot understand > if > > JS string is a very young or not. So Vitaly implemented an heuristics which > > checked how young string is (by looking where in new space it has been > > allocated---if it's too close to top, it's considered to be too young) and > > refused to externalize such strings. > > > > That's heuristics, not mandatory, but we may want to leave a room for it. > > > > Again, up to you to decide. > > Anton, the reason I think this "optimization" paid off in V8 is due to the fact > that weak handles were only handled during an old collection. The weak handles > for StringImpls should not be participating in grouping, so we can collect them > during new collections as well. Although I understand that there is an > additional benefit in your example of skipping the weak handle creation in the > first place.
Added an API function to determine the storage size. http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h File include/dart_api.h (right): http://codereview.chromium.org/11360114/diff/10001/include/dart_api.h#newcode... include/dart_api.h:1500: intptr_t length, Added an API function to determine the storage size of a string. On 2012/11/08 08:05:29, Anton Muhin wrote: > I might be missing something, sorry if it's the case. > > I meant that I found no embedder's API call to find out exact size of the > storage into which internal representation of VM string will fit. > > Yes, there is _IsAscii, but it's to some extent leaking internal implementation > to assume optimal_length = Dart_IsAscii ? Dart_StringLength : 2 * > Dart_StringLength. > > As an example, when 32 bits per code point were dropped, it might have affected > calculations. > > May we add a helper function to find out required length? > > Or do it in v8 way and allow the embedder to control internal representation of > the string? > > On 2012/11/07 21:43:27, siva wrote: > > This is the length of array passed in by the embedder. > > Since it creates the external space it would know the length. > > > > It can try and figure an appropriate length to be used by > > looking at Dart_StringLength and the type of the string > > (ascii or not). > > > > > > On 2012/11/07 10:05:46, Anton Muhin wrote: > > > how embedder can find out this length? > > >
lgtm and thanks a lot, Siva.
synched up to TOT.
synched up to TOT and resolved conflicts (hint hint).
LGTM with comments. -Ivan https://codereview.chromium.org/11360114/diff/25005/include/dart_api.h File include/dart_api.h (right): https://codereview.chromium.org/11360114/diff/25005/include/dart_api.h#newcod... include/dart_api.h:1495: DART_EXPORT Dart_Handle Dart_StringStorageSize(Dart_Handle str, intptr_t* size); I read the description and it is not clear to me whether this is the per indexed value storage size or the size needed to store this String. Maybe you can add an example how this is supposed to be used in conjunction with Dart_MakeExternalString? Basically should I do: intptr_t size; Dart_StringStorageSize(str, &size); void* data = malloc(size); or intptr_t len; Dart_StringLength(str, &len); intptr_t size; Dart_StringStorageSize(str, &size); void* data = malloc(size * len); https://codereview.chromium.org/11360114/diff/25005/vm/object.cc File vm/object.cc (right): https://codereview.chromium.org/11360114/diff/25005/vm/object.cc#newcode486 vm/object.cc:486: ASSERT(!obj.IsNull()); ASSERT(original_size >= used_size); https://codereview.chromium.org/11360114/diff/25005/vm/object.cc#newcode500 vm/object.cc:500: ((leftover_size - Array::InstanceSize(0)) / kWordSize); ASSERT(Array::InstanceSize(leftover_len) == leftover_size); https://codereview.chromium.org/11360114/diff/25005/vm/raw_object.h File vm/raw_object.h (right): https://codereview.chromium.org/11360114/diff/25005/vm/raw_object.h#newcode1215 vm/raw_object.h:1215: friend class String; I don't think this is needed any longer.
https://codereview.chromium.org/11360114/diff/25005/include/dart_api.h File include/dart_api.h (right): https://codereview.chromium.org/11360114/diff/25005/include/dart_api.h#newcod... include/dart_api.h:1495: DART_EXPORT Dart_Handle Dart_StringStorageSize(Dart_Handle str, intptr_t* size); Added comments to both Dart_StringStorageSize and Dart_MakeExternalString. On 2012/11/15 06:17:15, Ivan Posva wrote: > I read the description and it is not clear to me whether this is the per indexed > value storage size or the size needed to store this String. Maybe you can add an > example how this is supposed to be used in conjunction with > Dart_MakeExternalString? > > Basically should I do: > > intptr_t size; > Dart_StringStorageSize(str, &size); > void* data = malloc(size); > > or > > intptr_t len; > Dart_StringLength(str, &len); > intptr_t size; > Dart_StringStorageSize(str, &size); > void* data = malloc(size * len); https://codereview.chromium.org/11360114/diff/25005/vm/object.cc File vm/object.cc (right): https://codereview.chromium.org/11360114/diff/25005/vm/object.cc#newcode486 vm/object.cc:486: ASSERT(!obj.IsNull()); On 2012/11/15 06:17:15, Ivan Posva wrote: > ASSERT(original_size >= used_size); Done. https://codereview.chromium.org/11360114/diff/25005/vm/object.cc#newcode500 vm/object.cc:500: ((leftover_size - Array::InstanceSize(0)) / kWordSize); On 2012/11/15 06:17:15, Ivan Posva wrote: > ASSERT(Array::InstanceSize(leftover_len) == leftover_size); Done. https://codereview.chromium.org/11360114/diff/25005/vm/raw_object.h File vm/raw_object.h (right): https://codereview.chromium.org/11360114/diff/25005/vm/raw_object.h#newcode1215 vm/raw_object.h:1215: friend class String; On 2012/11/15 06:17:15, Ivan Posva wrote: > I don't think this is needed any longer. Done. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
