716716 - [CC] Add iterator over real nsPurpleBuffer entries

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

The purple buffer is a weird linked list of blocks where some elements are used and some aren't, depending whether a flag is set.  Iterating over the actual live elements in a purple buffer is pretty gross, and is done in nsPurpleBuffer::NoteAll, nsPurpleBuffer::SelectPointers and nsPurpleBuffer::UnmarkRemainingPurple, and another place will be added in bug 716518.  We should have some kind of real iterator.  I don't know if the way to go is to have some kind of iterator class like with NodePool and EdgePool, or we could use some kind of template magic.  The inner loops just take a pointer to a purple buffer entry.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: add iterator over live purple buffer entries in a block

Luke, how awful is the design of this iterator?  It is an iterator over an array, where we skip entries that don't have their low bit tagged.  I tried overloading the -> operator for it so we could just grab the mObject out of it without the fairly ugly GetNext, but then I wasn't sure how to go from an iterator to the underlying object, to pass it to the function Remove.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 590364 [details] [diff] [review]
add iterator over live purple buffer entries in a block

You could consider overloading both operator->() and operator*() where operator* returns an nsPurpleBufferEntry&.

Also: can aBlock->mEntries->mObject have the bit set?  If so, the constructor will need to do the same skip-until-not-set as operator++.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 590364 [details] [diff] [review]
> add iterator over live purple buffer entries in a block
> 
> You could consider overloading both operator->() and operator*() where
> operator* returns an nsPurpleBufferEntry&.

Yeah, I tried that, but doing Remove(i) still gave an error.  Maybe I'll try again in case I was doing something slightly wrong.

> Also: can aBlock->mEntries->mObject have the bit set?  If so, the
> constructor will need to do the same skip-until-not-set as operator++.

D'oh, I guess that's why it is crashing immediately...

Thanks!

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 590364 [details] [diff] [review]
add iterator over live purple buffer entries in a block

I messed around with a templatey implementation of iterating over purple buffer entries and it looks a lot nicer to me than this.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: add templated methods for iterating over purple buffer and purple buffer blocks

OO closures unfortunately make this a little ugly.

Comment 6

[Andrew McCreight [:mccr8]]

I have this blocking ICC, but I suppose I could just mash another function into the purple buffer that does yet another copy paste of the iteration code, so it isn't really blocking.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: add a template-based iterator

Comment 8

[Andrew McCreight [:mccr8]]

bonus code:
- removed the unused StaticAsserts()
- made the fields of nsPurpleBuffer private
- Fixed the adjustment of mCount in UnmarkRemainingPurple: before we were doing this for every entry, whether or not there was anything in it!  Totally bogus.  We should consider just crashing the browser if mCount != 0 which would let us tear out UnmarkRemainingPurple, which likely is never run ever, but I'm afraid of how that would go.

Otherwise the behavior should not be changed.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 742536 [details] [diff] [review]
add a template-based iterator

https://tbpl.mozilla.org/?tree=Try&rev=88a7945381c3

Comment 10

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d696aa2a413d

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d696aa2a413d