Issue 7082012: Heap profiler: fetch document.URL of global objects.

Issue 7082012: Heap profiler: fetch document.URL of global objects. (Closed)

Created:
9 years, 7 months ago by mnaganov (inactive)

Modified:
9 years, 6 months ago

Reviewers:
Søren Thygesen Gjesse, Vitaly Repeshko

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

Description

Heap profiler: fetch document.URL of global objects. This allows to distinguish DOMWindow objects in browser from each other. R=vitalyr@chromium.org,sgjesse@chromium.org BUG=https://bugs.webkit.org/show_bug.cgi?id=61177 TEST=cctest/test-heap-profiler/DocumentURL Committed: http://code.google.com/p/v8/source/detail?r=8111

Patch Set 1 #

Patch Set 2 : Style fix #

Total comments: 22

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+145 lines, -2 lines)			Patch
M	src/profile-generator.h	View		3 chunks	+4 lines, -0 lines	0 comments	Download
M	src/profile-generator.cc	View	1	7 chunks	+123 lines, -0 lines	20 comments	Download
M	test/cctest/test-heap-profiler.cc	View		2 chunks	+18 lines, -2 lines	2 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Vitaly Repeshko

LGTM with comments addressed. http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc File src/profile-generator.cc (right): http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#newcode1668 src/profile-generator.cc:1668: JSObject::cast(object))); nit: Fits on the ...

9 years, 6 months ago (2011-05-29 23:01:34 UTC) #2

mnaganov (inactive)

9 years, 6 months ago (2011-05-30 14:20:42 UTC) #4

Thanks for the comments!

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc
File src/profile-generator.cc (right):

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:1668: JSObject::cast(object)));
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> nit: Fits on the previous line?

Done.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2248: void VisitPointers(Object** start, Object** end)
{
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> nit: virtual

Done.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2270: void VisitPointers(Object** start, Object** end)
{
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> nit: virtual

Done.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2275: if (proxy->IsJSGlobalProxy()) {
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> Given that this loop has more conditions than its above twin, the tail of the
> handles array might be left uninitialized.

Fixed by eliminating the counter class.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2277: if (global->IsJSGlobalObject())
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> nit: {}

Done.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2290: Handle<JSGlobalObject>* objects_;
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> Can we use a growable collection of handles (e.g. list) and merge the two new
> visitors?

Sure. Sorry, I forgot about their existence.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2295: void V8HeapExplorer::TagGlobalObjects() {
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> Random idea. I don't know how flexible we want to be here, but in principle
> nothing prevents us from taking a context tagging script from the embedder. In
> Chromium it might be useful for non-window contexts.

Let's discuss it later.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2308: Handle<JSGlobalObject>& go =
enumerator.objects()[i];
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> Reference is not needed here. "go" is too short.

Removed ref. s/go/global_obj/

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2309: Object* obj_document =
go->GetPropertyNoExceptionThrown(*document_string);
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> This is not safe, because it asserts on failure/exception. Use something like:
> Object* obj_document;
> if (go->GetProperty(*document_string)->ToObject(&obj_document) &&
>     obj_document->IsJSObject()) {
>   JSObject* document = JSObject::cast(obj_document);
> }

Done.

http://codereview.chromium.org/7082012/diff/2001/src/profile-generator.cc#new...
src/profile-generator.cc:2312: Object* obj_url =
document->GetProperty(*url_string)->ToObjectUnchecked();
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> Also asserts on failure/exception.

Done.

http://codereview.chromium.org/7082012/diff/2001/test/cctest/test-heap-profil...
File test/cctest/test-heap-profiler.cc (right):

http://codereview.chromium.org/7082012/diff/2001/test/cctest/test-heap-profil...
test/cctest/test-heap-profiler.cc:1326: TEST(DocumentURL) {
On 2011/05/29 23:01:34, Vitaly Repeshko wrote:
> Please add a test where "document" and/or "URL" throw an exception.

Done.

Expand Messages | Collapse Messages