Closed
Bug 531711
Opened 15 years ago
Closed 13 years ago
imgLoader.cpp uses std::vector/algorithm
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: vlad, Unassigned)
References
Details
Attachments
(1 file, 2 obsolete files)
5.23 KB,
patch
|
joe
:
review-
|
Details | Diff | Splinter Review |
This shouldn't be using the stdlibc++ stuff, because it's not available everywhere. (Sad, but true.) I'd suggest switching this to use nsTArray. This is currently blocking building libxul on Android.
Reporter | ||
Comment 1•14 years ago
|
||
After some discussion, joe and I agree that nsTArray should get heap functions (MakeHeap/PushHeap/PopHeap); someone just needs to implement. Cc'ing mwu, he might have some cycles to work on this.
Updated•14 years ago
|
Assignee: nobody → mwu
Comment 2•14 years ago
|
||
So.. this appears to work but I have no idea really. Gonna write some tests. I wonder if it would be worth it to always add things to the heap properly through PushHeap instead of setting a dirty bit and rebuilding the heap.
Comment 3•14 years ago
|
||
New patch without the part adding heap functions, which is now in bug 537339. Otherwise mostly the same.
Attachment #419606 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #419721 -
Flags: review?(joe)
Comment 4•14 years ago
|
||
Adding things immediately makes us spend a bunch of time managing the heap while we're downloading images, because their weight changes a pile of times during the download. If we had some way of changing the weight of an element in the heap - which is possible in Fibonacci heaps, but not binomial, I believe - it wouldn't be necessary.
Comment 5•14 years ago
|
||
Ah I see. Would using fibonacci heaps in the future be worthwhile?
Comment 6•14 years ago
|
||
To simplify the imgLoader code, sure. But it's not performance-critical any more, thanks to the lazy update.
Comment 7•14 years ago
|
||
Some code simplified.
Attachment #419721 -
Attachment is obsolete: true
Attachment #435024 -
Flags: review?(joe)
Attachment #419721 -
Flags: review?(joe)
Comment 8•14 years ago
|
||
Comment on attachment 435024 [details] [diff] [review] Switch imgLoader.cpp to nsTArray, v2 >diff --git a/modules/libpr0n/src/imgLoader.cpp b/modules/libpr0n/src/imgLoader.cpp >+ PRUint32 index = mQueue.Length(); >+ nsRefPtr<imgCacheEntry> *elements = mQueue.Elements(); >+ >+ if (!index) >+ return; >+ >+ while (index--) { >+ if (elements[index] != entry) >+ continue; >+ mSize -= elements[index]->GetDataSize(); >+ mQueue.RemoveElementAt(index); Two separate comments: 1 - searching arrays backwards makes me sad :( 2 - We should just use nsTArray::IndexOf() instead of hand-rolling our own search method. >@@ -1289,14 +1283,11 @@ nsresult imgLoader::EvictEntries(imgCach >+ // We iterate backwards so a shift isn't required when >+ // RemoveFromCache removes the element from the queue. Can you reformat this so it's 80 characters wide? >+ imgCacheEntry *entry; >+ while ((entry = aQueueToClear.GetLast())) >+ if (!RemoveFromCache(entry)) > return NS_ERROR_FAILURE; I'd prefer something along the lines of entry = GetLast() while (entry) { do_stuff() entry = GetLast(); } >diff --git a/modules/libpr0n/src/imgLoader.h b/modules/libpr0n/src/imgLoader.h >-class imgCacheQueue [snip] >+class imgCacheQueue; Is there a reason you moved the definition of imgCacheQueue?
Attachment #435024 -
Flags: review?(joe) → review-
Reporter | ||
Comment 9•14 years ago
|
||
This no longer blocks android stuff, though I'm leaving it open since we may as well switch to our heap impl.
No longer blocks: android
Comment 10•13 years ago
|
||
Unassigning myself as there is little demand for this at the moment.
Assignee: mwu → nobody
Comment 11•13 years ago
|
||
We are now much less afraid of the STL than we used to be. I won't take a patch that changes this just to remove usage of STL.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•