Yarr performance improvements

RESOLVED FIXED in mozilla22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
This is mainly by looking at the diff between our yarr folder and webkit's. It consist of reserving the memory of vector before doing append. A second trick is using "swap" to get the contents of the vector, instead of copying it. (This is possible as the original vector isn't used anymore. We already did a similar trick for one of the two vectors, but instead used copy and clear).

This is good for 6% on v8-regexp.
(Assignee)

Comment 1

5 years ago
Created attachment 724270 [details] [diff] [review]
Patch

Annotate pointed into your direction, Dave. Feel free to nominate somebody else.
Assignee: general → hv1989
Attachment #724270 - Flags: review?(dmandelin)
(Assignee)

Updated

5 years ago
Blocks: 806646
Attachment #724270 - Flags: review?(dmandelin) → review+
(Assignee)

Comment 3

5 years ago
I'll investigate tomorrow, but awfy somehow doesn't show this 6% improvement ?!
(Assignee)

Comment 4

5 years ago
When measuring on my computer I took an average. And looks like before this commit v8-regexp was really jumping up and down here. With this patch it is dead stable. Somehow awfy didn't had this jumping and therefore also not the performance increase, booh, but it will definitely decrease the memory usage, yeay.
(Assignee)

Comment 5

5 years ago
Looks like octane-regexp did get this improvement, a 4% one :D

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/9f39547cfab3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Assignee)

Comment 7

5 years ago
I think this caused the crashes mentioned in bug 825243. Backout just to be sure.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1241c6d2ce
Depends on: 825243
Backout:
https://hg.mozilla.org/mozilla-central/rev/3e1241c6d2ce

However, we still hit the same crash after backing out, so I think you're OK to re-land this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

5 years ago
Created attachment 725359 [details] [diff] [review]
Use MOZ_CRASH when failing to allocate

Waldo suggested to use this for when we are failing allocating in Vector...
Attachment #725359 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #725359 - Flags: review? → review?(dmandelin)
(Assignee)

Updated

5 years ago
Attachment #725359 - Flags: review?(dmandelin) → review?(dvander)
(Assignee)

Comment 10

5 years ago
Because this wasn't the reason for the crashes, I pushed again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ea5daa2dc9
Whiteboard: [leave open]
Comment on attachment 725359 [details] [diff] [review]
Use MOZ_CRASH when failing to allocate

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

I guess this will be better than what's there, but I can guarantee we'll see OOM crash reports. Is there any way we can propagate OOM?
Attachment #725359 - Flags: review?(dvander) → review+
(Assignee)

Comment 12

5 years ago
Looks like we indeed see OOM crashes there. Original bug to propagate OOM is bug 574459 open since FF4.0 :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c41bf87b4e5
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/a7ea5daa2dc9
https://hg.mozilla.org/mozilla-central/rev/2c41bf87b4e5
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.