898692 - Wasted work in ArrayBufferObject::obj_trace()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[pchang9]

Attachment: Suggested patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release)
Build ID: 20130116073211

Steps to reproduce:

The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it.

In method main() in js/src/vm/TypedArrayObject.cpp, the loop on line 701 should break immediately after "found" is set to "true". All the iterations after "found" is set to "true" do not perform any useful work, at best they just set "found" again to "true".



Expected results:

Break loop after "found" is set to "true".

Comment 1

[[:Aleksej]]

> In method main() in js/src/vm/TypedArrayObject.cpp, the loop

It's about the DEBUG section in https://hg.mozilla.org/mozilla-central/file/18467a85acf6/js/src/vm/TypedArrayObject.cpp#l680

Comment 2

[Till Schneidereit [:till]]

This makes sense, yes. We don't care that much about DEBUG perf, but it's not entirely irrelevant, either.

@pchang9, would you mind looking at [1] to prepare a patch for checkin? If it's too much work, then don't worry - we can fix things up for you. In that case, we'd at least know which name and email address the patch should contain as author information.


[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 3

[pchang9]

Thanks for your information. Patch is prepared (898692.patch).

Comment 4

[pchang9]

Attachment: 898692.patch

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 783510 [details] [diff] [review]
898692.patch

Review of attachment 783510 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/TypedArrayObject.cpp
@@ +697,3 @@
>                          found = true;
> +                        break;
> +                    }

Actually, I'd prefer if you made this *slower*. :-)

Specifically, rather than breaking, can you JS_ASSERT(!found) just before setting found = true? That will check that each buffer shows up exactly once in the list (rather than at least once, as my slow version and your faster version did.)

Thanks!

Comment 6

[pchang9]

Attachment: 898692-1.patch

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 785391 [details] [diff] [review]
898692-1.patch

Review of attachment 785391 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!

Comment 8

[pchang9]

Attachment: 898692.patch (bug description updated, try testing)

Landed on try server:
https://tbpl.mozilla.org/?tree=Try&rev=3db32afd81f4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9852ea4110eb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9852ea4110eb

Comment 11

[Steve Fink [:sfink] [:s:]]

Great, thank you for this. Now you're ready to dive in deeper, right? :-)