Roy, during the crusade of bug 29358, we were bitten by one fundamental misunderstanding. I am including the full body of the function GetCMAP() to illustrate the problem and avoid further imbroglios. (It includes my changes for bug 73273, the addition of which led me to discover the problem -- but these changes have no effect in the illustration of the problem). To recap, here is the situation. Let's say we have a fully resolved (name0, map0) [in the hash] and a candidate (name1, map1) to be resolved. If name1==name0 then map1 is simply taken as map0 and the problem is solved. -> This is what happens upon entering GetCMAP(). So any subsequent checks like the ones you added later in the body of GetCMAP() is redundant and has no effect. Unfortunately we didn't catch this when reviewing. Erik's idea of the re-use was this: if name1 is not in the hash, then map1 is created. But if map1 appears to be identical to an existing map in the hash, then map1 is freed and a reference to the existing map is returned. --> On further reflexion, it looks like it might be okay if we relax this. Enumerating the hash to check for existence of a map might be a price too high to pay. So I would suggest just removing the redundant code -- i.e., add to the hash without the checks. PRUint32* nsFontMetricsWin::GetCMAP(HDC aDC, const char* aShortName, int* aFontType, PRUint8* aCharset) { if (!gInitializedFontMaps) { gInitializedFontMaps = 1; gFontMaps = PL_NewHashTable(0, HashKey, CompareKeys, nsnull, &fontmap_HashAllocOps, nsnull); if (!gFontMaps) { // error checking gInitializedFontMaps = 0; return nsnull; } gEmptyMap = (PRUint32*) PR_Calloc(2048, 4); if (!nsFontMetricsWin::gEmptyMap) { PL_HashTableDestroy(gFontMaps); gFontMaps = nsnull; gInitializedFontMaps = 0; return nsnull; } } nsString* name = new nsString(); if (!name) { return nsnull; } PRUint32* map; nsFontInfo* info; nsGetNameError ret = GetNAME(aDC, name); if (ret == eGetName_OK) { PLHashEntry **hep, *he; PLHashNumber hash = HashKey(name); hep = PL_HashTableRawLookup(gFontMaps, hash, name); he = *hep; if (he) { delete name; // an identical map has already been added info = NS_STATIC_CAST(nsFontInfo *, he); if (aCharset) { *aCharset = info->mCharset; } if (aFontType) { *aFontType = info->mType; } return info->mMap; } map = (PRUint32*) PR_Calloc(2048, 4); if (!map) { delete name; return nsnull; } } // GDIError occurs when we have raster font (not TrueType) else if (ret == eGetName_GDIError) { delete name; int charset = GetTextCharset(aDC); if (charset & (~0xFF)) { return gEmptyMap; } else { int j = gCharSetToIndex[charset]; //default charset is not dependable, skip it at this time if (j == eCharSet_DEFAULT) return gEmptyMap; PRUint32* charSetMap = gCharSetInfo[j].mMap; if (!charSetMap) { charSetMap = (PRUint32*) PR_Calloc(2048, 4); if (charSetMap) { gCharSetInfo[j].mMap = charSetMap; gCharSetInfo[j].GenerateMap(&gCharSetInfo[j]); } else { return gEmptyMap; } } if (aFontType) { *aFontType = NS_FONT_TYPE_UNICODE; } return charSetMap; } } else { // return an empty map, so that we never try this font again delete name; return gEmptyMap; } DWORD len = GetFontData(aDC, CMAP, 0, nsnull, 0); if ((len == GDI_ERROR) || (!len)) { delete name; PR_Free(map); return nsnull; } PRUint8* buf = (PRUint8*) PR_Malloc(len); if (!buf) { delete name; PR_Free(map); return nsnull; } DWORD newLen = GetFontData(aDC, CMAP, 0, buf, len); if (newLen != len) { PR_Free(buf); delete name; PR_Free(map); return nsnull; } PRUint8* p = buf + 2; PRUint16 n = GET_SHORT(p); p += 2; PRUint16 i; PRUint32 offset; for (i = 0; i < n; i++) { PRUint16 platformID = GET_SHORT(p); p += 2; PRUint16 encodingID = GET_SHORT(p); p += 2; offset = GET_LONG(p); p += 4; if (platformID == 3) { if (encodingID == 1) { // Unicode // Some fonts claim to be unicode when they are actually // 'pseudo-unicode' fonts that require a converter... // Here, we check if this font is a pseudo-unicode font that // we know something about, and we force it to be treated as // a non-unicode font. nsAutoString encoding; if (NS_SUCCEEDED(GetCharset(aShortName, encoding))) { PLHashEntry **hep, *he; PLHashNumber hash = HashKey(name); hep = PL_HashTableRawLookup(gFontMaps, hash, name); he = *hep; if (he) { // an identical map has already been added info = NS_STATIC_CAST(nsFontInfo *, he); if (aCharset) { *aCharset = info->mCharset; } if (aFontType) { *aFontType = info->mType; } return info->mMap; } // map didn't exist in the HashTable, let's add if (aCharset) { *aCharset = DEFAULT_CHARSET; } if (aFontType) { *aFontType = NS_FONT_TYPE_NON_UNICODE; } if (!GetMap(aShortName, map)) { PR_Free(map); map = gEmptyMap; } PR_Free(buf); he = PL_HashTableRawAdd(gFontMaps, hep, hash, name, nsnull); if (he) { info = NS_STATIC_CAST(nsFontInfo*, he); info->mType = NS_FONT_TYPE_NON_UNICODE; info->mMap = map; he->value = info; // so PL_HashTableLookup returns an nsFontInfo* return map; } delete name; if (map != gEmptyMap) PR_Free(map); return nsnull; } // if GetCharset(); break; // break out from for(;;) loop } //if (encodingID == 1) else if (encodingID == 0) { // symbol PLHashEntry **hep, *he; PLHashNumber hash = HashKey(name); hep = PL_HashTableRawLookup(gFontMaps, hash, name); he = *hep; if (he) { // an identical map has already been added info = NS_STATIC_CAST(nsFontInfo *, he); if (aCharset) { *aCharset = info->mCharset; } if (aFontType) { *aFontType = info->mType; } return info->mMap; } // map didn't exist in the HashTable, let's add if (aCharset) { *aCharset = SYMBOL_CHARSET; } if (aFontType) { *aFontType = NS_FONT_TYPE_NON_UNICODE; } if (!GetMap(aShortName, map)) { PR_Free(map); map = gEmptyMap; } PR_Free(buf); he = PL_HashTableRawAdd(gFontMaps, hep, hash, name, nsnull); if (he) { info = NS_STATIC_CAST(nsFontInfo*, he); info->mCharset = SYMBOL_CHARSET; info->mType = NS_FONT_TYPE_NON_UNICODE; info->mMap = map; he->value = info; // so PL_HashTableLookup returns an nsFontInfo* return map; } delete name; if (map != gEmptyMap) PR_Free(map); return nsnull; } } // if (platformID == 3) } // for loop if (i == n) { PR_Free(buf); delete name; PR_Free(map); return nsnull; } p = buf + offset; PRUint16 format = GET_SHORT(p); if (format != 4) { PR_Free(buf); delete name; PR_Free(map); return nsnull; } PRUint8* end = buf + len; // XXX byte swapping only required for little endian (ifdef?) while (p < end) { PRUint8 tmp = p[0]; p[0] = p[1]; p[1] = tmp; p += 2; } PRUint16* s = (PRUint16*) (buf + offset); PRUint16 segCount = s[3] / 2; PRUint16* endCode = &s[7]; PRUint16* startCode = endCode + segCount + 1; PRUint16* idDelta = startCode + segCount; PRUint16* idRangeOffset = idDelta + segCount; PRUint16* glyphIdArray = idRangeOffset + segCount; static int spacesInitialized = 0; static PRUint32 spaces[2048]; if (!spacesInitialized) { spacesInitialized = 1; SET_SPACE(0x0020); SET_SPACE(0x00A0); for (PRUint16 c = 0x2000; c <= 0x200B; c++) { SET_SPACE(c); } SET_SPACE(0x3000); } PRUint32 maxGlyph; PRUint8* isSpace = GetSpaces(aDC, &maxGlyph); if (!isSpace) { PR_Free(buf); delete name; PR_Free(map); return nsnull; } for (i = 0; i < segCount; i++) { if (idRangeOffset[i]) { PRUint16 startC = startCode[i]; PRUint16 endC = endCode[i]; for (PRUint32 c = startC; c <= endC; c++) { PRUint16* g = (idRangeOffset[i]/2 + (c - startC) + &idRangeOffset[i]); if ((PRUint8*) g < end) { if (*g) { PRUint16 glyph = idDelta[i] + *g; if (glyph < maxGlyph) { if (isSpace[glyph]) { if (SHOULD_BE_SPACE(c)) { ADD_GLYPH(map, c); } } else { ADD_GLYPH(map, c); } } } } else { // XXX should we trust this font at all if it does this? } } //printf("0x%04X-0x%04X ", startC, endC); } else { PRUint16 endC = endCode[i]; for (PRUint32 c = startCode[i]; c <= endC; c++) { PRUint16 glyph = idDelta[i] + c; if (glyph < maxGlyph) { if (isSpace[glyph]) { if (SHOULD_BE_SPACE(c)) { ADD_GLYPH(map, c); } } else { ADD_GLYPH(map, c); } } } //printf("0x%04X-0x%04X ", startCode[i], endC); } } //printf("\n"); PR_Free(buf); PR_Free(isSpace); if (aCharset) { *aCharset = DEFAULT_CHARSET; } if (aFontType) { *aFontType = NS_FONT_TYPE_UNICODE; } PLHashEntry **hep, *he; PLHashNumber hash = HashKey(name); hep = PL_HashTableRawLookup(gFontMaps, hash, name); he = *hep; if (he) { // an identical map has already been added info = NS_STATIC_CAST(nsFontInfo *, he); if (aCharset) { *aCharset = info->mCharset; } if (aFontType) { *aFontType = info->mType; } return info->mMap; } he = PL_HashTableRawAdd(gFontMaps, hep, hash, name, nsnull); if (he) { info = NS_STATIC_CAST(nsFontInfo*, he); he->value = info; // so PL_HashTableLookup returns an nsFontInfo* info->mMap = map; return map; } delete name; if (map != gEmptyMap) PR_Free(map); return nsnull; }

FYI, each of these maps is 8K -- so not sharing them isn't that bad, and can be revisited if they show up on the perf radar, or if it becomes a concern for embedders. Until then, the status quo now seems okay to me.

rbs< Thanks. :) We will revisit once on the radar.

Yeah, but removing the unneeeded code is still a Good Thing (TM) -- it will shave off some cycles.

>So I would suggest just removing the redundant code -- i.e., add to the hash without the checks. Which part is this "redundant code" ? >FYI, each of these maps is 8K -- so not sharing them isn't that bad, We don't want to INCREASE the memory footprint now, right ? Gecko already use too much memory

Frank, I guess you may have mis-read the bug. The current code is not doing what it is supposed to be doing, so it is not helping to save memory. On the contrary, it is eating some cycles. That's why I noted that it is better to remove it than to leave it there. The problem with the real fix is that it involves enumerating the whole hash to check for a matching map. So there is yet another space-speed dilemma here. Which one to pick? The perf radars may help. Because it is yet unclear what to do, if the real fix has to be implemented, then maybe another bug should be filed. This one is an easy fix (it involves just removing the unneeded lines of code).

For the record - I filed the separate bug 75260: Investigate the sharing of font maps

Focus on bug fix

Since perf is important at this stage, I am adding the perf keyword and taking the bug (it is a 'bug' that folks care about because it contributes in making Mozilla sluggish than necessary). I am wandering into that code at moment due to the work that I am doing for bug 73273. So I might as well grab this low hanging fruit.

I replaced the dead code with an assertion, and as expected I haven't hit the assertion so far. I may remove that #ifdef'ned section before checkin since it really has little use. Seeking r= and sr= to move on, thanks.

I think the patch will crash, if I'm mentally applying it correctly, in the non-NS_DEBUG case: - PLHashEntry **hep, *he; + PLHashEntry **hep = NULL, *he; PLHashNumber hash = HashKey(name); +#ifdef NS_DEBUG . . . + NS_ASSERTION(!*hep, "Something is wrong somewhere"); +#endif if (aCharset) { *aCharset = DEFAULT_CHARSET; } @@ -1272,6 +1262,8 @@ } PR_Free(buf); + // XXX Need to check if an identical map has already been added + // XXX See Bug 75260 "Investigate the sharing of font maps" he = PL_HashTableRawAdd(gFontMaps, hep, hash, name, nsnull); PL_HashTableRawAdd requires a non-null hep parameter. See http://lxr.mozilla.org/mozilla/source/nsprpub/lib/ds/plhash.c#278. /be

Ready to go, has been baking in my tree for quite a while now with no glitches. Seeking r=shanjian and sr=brendan to get one more bug off the radar, thanks.

Looks good to me. sr=brendan@mozilla.org. Notes for future hacking: - else after return in the eGetName_GDIError, else after break later, all make it harder than necessary to see that early returns do what they should: avoid letting control flow fall into code that must not be reached. - instead of allocating an nsString for the lookup, use an nsAutoString, and clone it into an nsString only on gFontMaps "cache miss". Hits outnumber misses typically. If they don't here, never mind. /be

Yep, a rewrite could help that code. I even lobbied for that to happen (bug 45737) but have been unsuccessful so far :-) Even the present bug barely escaped the danger of being dropped in the boundless "Future" bucket.

Checked-in. Resolving as FIXED. One more off the radars.

Marking verified per last comments.