Tighten up the assertions in MapAlignedPages a bit

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
I used MapAlignedPages to quickly test something today and realized that it asserts if you pass in an alignment smaller than the allocation granularity. It seems to me there's no need for this - getting back a region that's more aligned than requested is fine, so long as the allocation granularity is a multiple of the requested alignment.

On the other hand, we don't assert if |size| is smaller than the allocation granularity. Windows in particular doesn't do mappings smaller than 64KiB, so asking for anything smaller is wasteful.
(Assignee)

Comment 1

2 years ago
Created attachment 8789477 [details] [diff] [review]
Fix assertions in MapAlignedPages to make it more intuitive.

This changes the assertions as described above (and adds a couple of related comments to AllocateMappedContent).

The patch is very simple and MapAlignedPages only has 2 callers, both of which obviously obey the rules, so I think this somewhat anemic build-only try run should be fine:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f03d1862457
Attachment #8789477 - Flags: review?(terrence)
Comment on attachment 8789477 [details] [diff] [review]
Fix assertions in MapAlignedPages to make it more intuitive.

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

Thanks!
Attachment #8789477 - Flags: review?(terrence) → review+
(Assignee)

Comment 3

2 years ago
And thank *you* for the fast review :)
Keywords: checkin-needed

Comment 4

2 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa4bfef55aa
Fix assertions in MapAlignedPages to make it more intuitive. r=terrence CLOSED TREE
Keywords: checkin-needed

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cfa4bfef55aa
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.