non-minimum-sized GC arena pools have wrong alignment modulus

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: brendan, Assigned: brendan)

Tracking

({js1.6, verified1.8.0.2, verified1.8.1})

Trunk
mozilla1.9alpha1
js1.6, verified1.8.0.2, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nvn-dl])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
See bug 294499 -- thanks to Igor for noticing.  Patch in a trice.

/be
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
(Assignee)

Comment 1

12 years ago
Created attachment 208562 [details] [diff] [review]
minimal fix

Want to get this into the 1.8x branches in due course, and into the JS1.6 RC1 minibranch.

/be
Attachment #208562 - Flags: review?(igor.bukanov)
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1+
Flags: blocking1.8.0.2+

Comment 2

12 years ago
Comment on attachment 208562 [details] [diff] [review]
minimal fix

Why using sizeof(JSGCThing) and not 1 given that GC code aligns things itself?

Comment 3

12 years ago
(In reply to comment #1)
> Created an attachment (id=208562) [edit]
> minimal fix
> 
> Want to get this into the 1.8x branches in due course, and into the JS1.6 RC1
> minibranch.

This bug has a consequence that prevented mixing of arenas for things of different sizes. For example, if an arena was allocated for things with size 5*sizeof(JSGCThing) and later freed, it would only be re-used for things of size 5..8 and not for sizeof(JSGCThings) (objects, strings or doubles).

So fixing this bug can theoretically expose GC-related bugs that currently not visible as, for example, xml things do not mix with strings.
(Assignee)

Comment 4

12 years ago
Created attachment 208568 [details] [diff] [review]
minimal fix, v2

Good point, the arena alignment machinery is unused here.

Yes, we'll get more recycling via arena_freelist than before, because it does exact fit and sizes are no longer different among freelist pools.  I'm not sure how afraid to be of this.  It seems worth trying out for a JS1.6 RC1.  I wouldn't put it in 1.8.0.1, but it might fit in 1.8.0.2.

/be
Attachment #208562 - Attachment is obsolete: true
Attachment #208568 - Flags: review?(igor.bukanov)
Attachment #208562 - Flags: review?(igor.bukanov)

Updated

12 years ago
Attachment #208568 - Flags: review?(igor.bukanov) → review+

Comment 5

12 years ago
patch me baby! Regressions are my reason to live!
(Assignee)

Comment 6

12 years ago
Comment on attachment 208568 [details] [diff] [review]
minimal fix, v2

This is going into the trunk now, so it can be picked up before Igor's major trunk-only GC changes land.

/be
Attachment #208568 - Flags: approval1.8.1?
Attachment #208568 - Flags: approval1.8.0.2?
(Assignee)

Comment 7

12 years ago
Fixed in the trunk:

$ cvs ci -m"Remove bogus arena-pool alignment parameters (323529, r=igor)." jsgc.c
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.112; previous revision: 3.111
done

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Attachment #208568 - Flags: approval1.8.1? → branch-1.8.1?(brendan)
(Assignee)

Comment 8

12 years ago
Comment on attachment 208568 [details] [diff] [review]
minimal fix, v2

Who is going to check all these branch-1.8.1+'d patches in?

/be
Attachment #208568 - Flags: branch-1.8.1?(brendan)
Attachment #208568 - Flags: branch-1.8.1+
Attachment #208568 - Flags: approval1.8.0.2?
Attachment #208568 - Flags: approval1.8.0.2+

Updated

11 years ago
Flags: testcase-
(Assignee)

Comment 9

11 years ago
Fixed on branches.

/be
Keywords: fixed1.8.0.2, fixed1.8.1
Please provide a testcase and testing guidance so that we can verify this bug in the Firefox 1.5.0.2 candidate builds.
Whiteboard: [tcn-dl]
(Assignee)

Comment 11

11 years ago
This isn't testable in any practical way.  The bug was found by inspection, it wasted real but small amounts of memory, but the noise and other signals in our dynamic footprint dwarf those amounts.

Verified by code inspection.

/be
Status: RESOLVED → VERIFIED
marking [nvn-dl], which removes this bug from the "to be verified by QA" list
for Firefox 1.5.0.2.
Whiteboard: [tcn-dl] → [nvn-dl]

Comment 13

11 years ago
2006-02-22 15:40 mozilla/js/src/jsgc.c 	3.104.2.3.2.2 MOZILLA_1_8_0_BRANCH
2006-02-22 15:39 mozilla/js/src/jsgc.c 	3.104.2.5     MOZILLA_1_8_BRANCH
Keywords: fixed1.8.0.2, fixed1.8.1 → verified1.8.0.2, verified1.8.1
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.