Closed Bug 1271407 Opened 4 years ago Closed 4 years ago

Remove unnecessary methods from `js::ScriptSource`

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: fitzgen, Assigned: fitzgen)

Details

Attachments

(1 file, 1 obsolete file)

This commit removes the following methods from `js::ScriptSource`:

* `compressedChars`
* `compressedBytes`
* `uncompressedChars`

They are throwbacks from when `ScriptSource` had this big, hand-rolled tagged
union, and these methods were getters that did all sanity asserts on the tag
state. That union has since been replaced with a `mozilla::Variant`, so we
should just use `mozilla::Variant`'s type-safe accessors instead.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13ffa6beb734
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Attachment #8750441 - Flags: review?(luke) → review+
holy crap, this set of changes caused a boatload of talos regressions, here is the list of improvements when we back this out:
https://treeherder.mozilla.org/perf.html#/alerts?id=1152
(In reply to Joel Maher (:jmaher) from comment #5)
> holy crap, this set of changes caused a boatload of talos regressions, here
> is the list of improvements when we back this out:
> https://treeherder.mozilla.org/perf.html#/alerts?id=1152

I think this is much more likely caused by the patch in bug 1269451, which landed at the same time.
Try push and talos run with just this commit:

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9700b496e01
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=d9700b496e01
This fixes one more use of `uncompressedChars` that went away in bug 1269451,
but needs to go away in this patch now that I'm rebasing off of bug 1269451.

Carrying r+.
Attachment #8752404 - Flags: review+
Attachment #8750441 - Attachment is obsolete: true
Try push:

remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aeec30df380
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=7aeec30df380
As suspected, perfherder shows that this patch is fine, and it was the other bug that caused the slow downs.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6c25ee373223
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.