Provide a way for iterating through all external strings referenced from the JS heap

Provide a way for iterating through all external strings referenced from the JS heap

Committed: http://code.google.com/p/v8/source/detail?r=10400

[mnaganov (inactive), 2012-01-11 18:50:22]

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h
File include/v8-profiler.h (right):

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode442
include/v8-profiler.h:442: class ExternalResourceVisitor {
You need to put V8EXPORT before the class name.

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode445
include/v8-profiler.h:445: virtual ~ExternalResourceVisitor() {}
Destructor must be the first.

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode452
include/v8-profiler.h:452: static void
VisitExternalResources(ExternalResourceVisitor* visitor);
I'm not sure HeapProfiler class is a good place for this method -- it doesn't
relate to HeapSnapshots at all, as it iterates live heap. Why not to add it into
the V8 interface class?

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.cc
File src/heap-profiler.cc (right):

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.cc#newcode112
src/heap-profiler.cc:112: if (!obj->IsString())
Why not obj->IsExternalString()?

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h
File src/heap-profiler.h (right):

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode76
src/heap-profiler.h:76: class ExternalResourceVisitor {
Do you really need this class? What's wrong with using the one you have defined
in the API?

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode81
src/heap-profiler.h:81: static void
VisitExternalResources(ExternalResourceVisitor* visitor);
The same is for the implementation. Perhaps, put this into heap.cc?

http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:860: static const size_t max_length = 100;
kMaxLength

http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:890: if (resource1_ == resource)
Perhaps, just use boolean flags? Modifying references looks too hackish.

[yurys, 2012-01-11 19:50:13]

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h
File include/v8-profiler.h (right):

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode442
include/v8-profiler.h:442: class ExternalResourceVisitor {
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> You need to put V8EXPORT before the class name.

Done.

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode445
include/v8-profiler.h:445: virtual ~ExternalResourceVisitor() {}
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> Destructor must be the first.

Done.

http://codereview.chromium.org/9139018/diff/1/include/v8-profiler.h#newcode452
include/v8-profiler.h:452: static void
VisitExternalResources(ExternalResourceVisitor* visitor);
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> I'm not sure HeapProfiler class is a good place for this method -- it doesn't
> relate to HeapSnapshots at all, as it iterates live heap. Why not to add it
into
> the V8 interface class?

Since it is used to analyze heap I decided to put it into the heap profiler. I
don't mind moving it into V8 class if you feel it should rather go there.

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.cc
File src/heap-profiler.cc (right):

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.cc#newcode112
src/heap-profiler.cc:112: if (!obj->IsString())
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> Why not obj->IsExternalString()?

Done.

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h
File src/heap-profiler.h (right):

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode76
src/heap-profiler.h:76: class ExternalResourceVisitor {
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> Do you really need this class? What's wrong with using the one you have
defined
> in the API?
Done. I thought we try to work only with internal classes in the code under src/

http://codereview.chromium.org/9139018/diff/1/src/heap-profiler.h#newcode81
src/heap-profiler.h:81: static void
VisitExternalResources(ExternalResourceVisitor* visitor);
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> The same is for the implementation. Perhaps, put this into heap.cc?

Done.

http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler.cc
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:860: static const size_t max_length = 100;
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> kMaxLength

Done.

http://codereview.chromium.org/9139018/diff/1/test/cctest/test-heap-profiler....
test/cctest/test-heap-profiler.cc:890: if (resource1_ == resource)
On 2012/01/11 18:50:23, Mikhail Naganov (Chromium) wrote:
> Perhaps, just use boolean flags? Modifying references looks too hackish.

Done.

[mnaganov (inactive), 2012-01-12 10:37:27]

Looks good to me, but since it changes the V8 API, I would like to ask Daniel
for the final approval.

http://codereview.chromium.org/9139018/diff/4001/src/api.cc
File src/api.cc (left):

http://codereview.chromium.org/9139018/diff/4001/src/api.cc#oldcode6032
src/api.cc:6032: 
nit: please, leave 2 lines here, too

http://codereview.chromium.org/9139018/diff/4001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/9139018/diff/4001/src/api.cc#newcode4050
src/api.cc:4050: 
nit: 2 empty lines between function definitions.

[yurys, 2012-01-12 12:00:06]

Daniel, could you take a look?

http://codereview.chromium.org/9139018/diff/4001/src/api.cc
File src/api.cc (left):

http://codereview.chromium.org/9139018/diff/4001/src/api.cc#oldcode6032
src/api.cc:6032: 
On 2012/01/12 10:37:27, Mikhail Naganov (Chromium) wrote:
> nit: please, leave 2 lines here, too

Done.

http://codereview.chromium.org/9139018/diff/4001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/9139018/diff/4001/src/api.cc#newcode4050
src/api.cc:4050: 
On 2012/01/12 10:37:27, Mikhail Naganov (Chromium) wrote:
> nit: 2 empty lines between function definitions.

Done.

[Yang, 2012-01-13 07:55:34]

On 2012/01/12 12:00:06, Yury Semikhatsky wrote:
> Daniel, could you take a look?
> 
> http://codereview.chromium.org/9139018/diff/4001/src/api.cc
> File src/api.cc (left):
> 
> http://codereview.chromium.org/9139018/diff/4001/src/api.cc#oldcode6032
> src/api.cc:6032: 
> On 2012/01/12 10:37:27, Mikhail Naganov (Chromium) wrote:
> > nit: please, leave 2 lines here, too
> 
> Done.
> 
> http://codereview.chromium.org/9139018/diff/4001/src/api.cc
> File src/api.cc (right):
> 
> http://codereview.chromium.org/9139018/diff/4001/src/api.cc#newcode4050
> src/api.cc:4050: 
> On 2012/01/12 10:37:27, Mikhail Naganov (Chromium) wrote:
> > nit: 2 empty lines between function definitions.
> 
> Done.

Drive-by:
As far as I can tell, every external string is registered to the external string
table (class ExternalStringTable) when it's created and removed from it when
it's finalized (Heap::UpdateNewSpaceReferencesInExternalStringTable). Wouldn't
it be easier and more efficient to iterate over this table instead of checking
every object?

[yurys, 2012-01-13 12:31:52]

On 2012/01/13 07:55:34, Yang wrote:
> On 2012/01/12 12:00:06, Yury Semikhatsky wrote:
> Drive-by:
> As far as I can tell, every external string is registered to the external
string
> table (class ExternalStringTable) when it's created and removed from it when
> it's finalized (Heap::UpdateNewSpaceReferencesInExternalStringTable). Wouldn't
> it be easier and more efficient to iterate over this table instead of checking
> every object?

Good point, thanks! Done.

[danno, 2012-01-13 15:00:04]

LGTM with comments. Please make it clear from the documentation that this API
cannot be relied upon to be high-performance, despite Yang's suggestion. Also
note that you probably want to add a visitor method for registered external
arrays (i.e. buffers registered through
SetIndexedPropertiesToExternalArrayData). Doesn't have to be in this CL, though.

[yurys, 2012-01-13 15:14:27]

On 2012/01/13 15:00:04, danno wrote:
> LGTM with comments. Please make it clear from the documentation that this API
> cannot be relied upon to be high-performance, despite Yang's suggestion.
Done.

> Also note that you probably want to add a visitor method for registered
external
> arrays (i.e. buffers registered through
> SetIndexedPropertiesToExternalArrayData). Doesn't have to be in this CL,
though.
Yes, I was going to add it later once we have a client scenario for it in
Chromium.

[Tobias Burnus, 2012-01-13 20:57:01]

Unfortunately, the test included in patch breaks with GCC 4.7 with a "set but
not used" warning.

http://codereview.chromium.org/9139018/diff/14001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/9139018/diff/14001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:13723: v8::Local<v8::String> string2 =
v8::String::NewExternal(resource2);
The assignment to string1 and string2 breaks the build with GCC 4.7 as it
rightly warns:

../test/cctest/test-api.cc: In function 'void TestVisitExternalStrings()':
../test/cctest/test-api.cc:13721:25: error: variable 'string1' set but not used
[-Werror=unused-but-set-variable]
../test/cctest/test-api.cc:13723:25: error: variable 'string2' set but not used
[-Werror=unused-but-set-variable]

[yurys, 2012-01-14 13:52:44]

http://codereview.chromium.org/9139018/diff/14001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/9139018/diff/14001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:13723: v8::Local<v8::String> string2 =
v8::String::NewExternal(resource2);
On 2012/01/13 20:57:01, Tobias Burnus wrote:
> The assignment to string1 and string2 breaks the build with GCC 4.7 as it
> rightly warns:
> 
> ../test/cctest/test-api.cc: In function 'void TestVisitExternalStrings()':
> ../test/cctest/test-api.cc:13721:25: error: variable 'string1' set but not
used
> [-Werror=unused-but-set-variable]
> ../test/cctest/test-api.cc:13723:25: error: variable 'string2' set but not
used
> [-Werror=unused-but-set-variable]
> 
I can add an artificial usages of string1 and string2 but the failure sounds
wrong since v8::Local protects the strings from being GC'ed. I don't see why I
cannot have assigned but never used scoped_ptr as a method local variable. What
is recommended way of dealing with such warnings in v8? Btw, is there a build
bot using GCC4.7 where I could see the failure?

[Tobias Burnus, 2012-01-14 14:27:40]

On 2012/01/14 13:52:44, Yury Semikhatsky wrote:
> I can add an artificial usages of string1 and string2 but the failure sounds
> wrong since v8::Local protects the strings from being GC'ed. I don't see why I
> cannot have assigned but never used scoped_ptr as a method local variable.

Well, language wise is allowed - thus the compiler only warns. The warning is
GCC new since 4.7.

However, as v8 is compiled with -Wall (which implies in GCC 4.7
-Werror=unused-but-set-variable) and with -Werror (treat warnings as error),
this will lead to build failures with GCC 4.7.

The warning makes sense as unused variables usually indicate dead code (and
sometimes even algorithmic bugs).


> What is recommended way of dealing with such warnings in v8?

For real code: - usually - to remove those variables. For test code: No idea.
Removing the variables? Adding an artificial use? Changing the compiler options?


> Btw, is there a build bot using GCC4.7 where I could see the failure?

I don't think that there is already a GCC 4.7 bot available. Nevertheless, I
would appreciate if one could build v8 with GCC 4.7.

Regarding the failure, the message is a simple:
test-api.cc:13721:25: error: variable 'string1' set but not used


Tobias,
who is a mere user of v8 and cannot provide any official guidance