116150 - review compressed charmap changes for FreeType checkin

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[kill this account]

this is to help manage the checkin of the compressed charmap changes for the
FreeType2 code

Comment 1

[kill this account]

Attachment: patch; nsCompressedCharMap changes

Comment 2

[Shanjian Li]

brian,
One question. if (mid[j] != CCMAP_EMPTY_PAGE), does that mean the 
page is non-empty? Do you have to check it again?

Comment 3

[Shanjian Li]

reassign back to bstell.

Comment 4

[kill this account]

> One question. if (mid[j] != CCMAP_EMPTY_PAGE), does that mean the
> page is non-empty? Do you have to check it again?

In general the mid pointers will point to the empty page when the page
is empty. But it is possible that a bit was set then cleared. This
could leave the page empty but the mid pointer not pointing to the
empty page.

Comment 5

[Shanjian Li]

brian,
I am OK with your patch. r= shanjia, and you can go ahead. 

However, I am a little worried about CCMAP_UNSET_CHAR. (I am not sure if you 
intend to add CCMAP_SET_CHAR in future.) To operate directly on CCMAP may 
make a CCMAP less "compact", and it is not safe for all-bit-set page. My 
IsSameCCMap function rely on the ccmap to be most "compact".  

Comment 6

[kill this account]

I agree that there should never have a CCMAP_SET_CHAR macro.

Nice catch: thanks for pointing out the problem about CCMAP_UNSET_CHAR and
the all-bit-set page. This should be fixed.

Comment 7

[Brendan Eich [:brendan]]

Should I await a new patch?  Please cc: me on bugs you want me to sr=, in
addition to mailing me.  Thanks.

/be

Comment 8

[kill this account]

At the current moment unset is only used in one place in 
nsFontMetrics{GTK,Xlib}.cpp and it is not a problem. I do not know the correct
answer for unsetting yet so I've opened bug 118757 to work on this. 

When Xrender (better performance where avail) is added to the TrueType work
I will need a way to remember which chars/glyphs have been uploaded to the
X server. My current thinking is to make a "glyphs not sent yet" ccmap 
(by dup'ing the ccmap) and unset the bits as the glyphs are sent up.

I would like to think on these for a while.

In the meantime I would prefer to move forward on this patch.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 62305 [details] [diff] [review]
patch; nsCompressedCharMap changes

>+PRBool
>+NextNonEmptyCCMapPage(PRUint16* aCCMap, PRUint16 &aPageStart)

Ewww, a reference parameter.  I looked at the call to this function first
(reading the patch backward or top-down) and couldn't see the side effect
except by noticing that the actual parameter must have been modified by the
call to this function.	That's why I dislike reference params (and that's the
only reason; I believe the only reason they're in C++ in the first place is for
overloaded operators -- stroustrup's original Complex class).

>+  if (aPageStart == CCMAP_BEGIN_AT_START_OF_MAP) {
>+    upper_index = 0;
>+    mid_index   = 0;
>+  }
>+  else {
>+    upper_index = CCMAP_UPPER_INDEX(aPageStart);
>+    mid_index   = CCMAP_MID_INDEX(aPageStart) + 1;
>+  }

Could you avoid this if-else, using just the else clause, with the following
tweak?

  upper_index = CCMAP_UPPER_INDEX(aPageStart);
  mid_index   = CCMAP_MID_INDEX(aPageStart + PR_BIT(CCMAP_BITS_PER_PAGE_LOG2));

if you #define CCMAP_BEGIN_AT_START_OF_MAP as follows?

#define CCMAP_BEGIN_AT_START_OF_MAP \
  (PR_BITMASK(CCMAP_BITS_PER_MID_LOG2) << CCMAP_BITS_PER_PAGE_LOG2)

>+  // walk thru the upper pointers
>+  PRUint16 *upper = &aCCMap[0];
>+  for (i=upper_index; i<CCMAP_NUM_UPPER_POINTERS; i++) {
>+    if (upper[i] == CCMAP_EMPTY_MID) {
>+      mid_index = 0;

Put mid_index = 0 with i++ in the for-loop control, to share it in source and
compiled (optimizers may do it for you, even if you state it twice as in this
patch) code.

>+      continue;
>+    }
>+
>+    // walk the mid array
>+    PRUint16 *mid = &aCCMap[upper[i]];
>+    for (j=mid_index; j<CCMAP_NUM_MID_POINTERS; j++) {
>+      if (mid[j] == CCMAP_EMPTY_PAGE)
>+        continue;
>+
>+      // walk the page
>+      ALU_TYPE *page = (ALU_TYPE*)&aCCMap[mid[j]];

I forget whether we're aligning all &aCCMap[mid[j]] on an ALU_TYPE boundary --
can you refresh my memory?  Thanks.

>+      for (k=0; k<CCMAP_NUM_ALUS_PER_PAGE; k++) {
>+        if (page[k] != 0) {
>+          PRUint32 base = (i*CCMAP_NUM_UCHARS_PER_MID) + (j*CCMAP_NUM_UCHARS_PER_PAGE);
>+          NS_ASSERTION(base<NUM_UNICODE_CHARS, "invalid page address");
>+          aPageStart = (PRUint16)base;
>+          return PR_TRUE;
>+        }
>+      }
>+    }
>+    mid_index = 0;

Here's the other mid_index = 0 that could go in the loop control's update part.

sr=[e-mail] if you consider the above and adopt what seems good.

/be

Comment 10

[kill this account]

> >+PRBool
> >+NextNonEmptyCCMapPage(PRUint16* aCCMap, PRUint16 &aPageStart)
> 
> Ewww, a reference parameter. 

I will change this to a pointer.

> >+  if (aPageStart == CCMAP_BEGIN_AT_START_OF_MAP) {
> >+    upper_index = 0;
> >+    mid_index   = 0;
> >+  }
> >+  else {
> >+    upper_index = CCMAP_UPPER_INDEX(aPageStart);
> >+    mid_index   = CCMAP_MID_INDEX(aPageStart) + 1;
> >+  }
> 
> Could you avoid this if-else, using just the else clause, with the following
> tweak?
> 
>   upper_index = CCMAP_UPPER_INDEX(aPageStart);
>   mid_index   = CCMAP_MID_INDEX(aPageStart + 
PR_BIT(CCMAP_BITS_PER_PAGE_LOG2));
> 
> if you #define CCMAP_BEGIN_AT_START_OF_MAP as follows?
> 
> #define CCMAP_BEGIN_AT_START_OF_MAP \
>   (PR_BITMASK(CCMAP_BITS_PER_MID_LOG2) << CCMAP_BITS_PER_PAGE_LOG2)

I'm a bit unclear here on this. Wouldn't this make 
CCMAP_BEGIN_AT_START_OF_MAP have a lot of bits set in the
UPPER_INDEX at the start (instead of 0 bits set)?

If I changed this to:

  #define CCMAP_BEGIN_AT_START_OF_MAP (0 - CCMAP_NUM_UCHARS_PER_PAGE)

and 

  *aPageStart += CCMAP_NUM_UCHARS_PER_PAGE;
  upper_index = CCMAP_UPPER_INDEX(aPageStart);
  mid_index   = CCMAP_MID_INDEX(aPageStart);

I still need a end test to distinguish between
0x0 and 0x10000.

> Put mid_index = 0 with i++ in the for-loop control, to share it in source and
> compiled (optimizers may do it for you, even if you state it twice as in this
> patch) code.

Nice. Thanks.

> I forget whether we're aligning all &aCCMap[mid[j]] on an ALU_TYPE boundary --
> can you refresh my memory?  Thanks.

Pages are aligned on 16 byte boundries (from the beginning of
the map).

Comment 11

[Brendan Eich [:brendan]]

>> Could you avoid this if-else, using just the else clause, with the following
>> tweak?
>>   upper_index = CCMAP_UPPER_INDEX(aPageStart);
>>   mid_index   = CCMAP_MID_INDEX(aPageStart + PR_BIT(CCMAP_BITS_PER_PAGE_LOG2));
>> if you #define CCMAP_BEGIN_AT_START_OF_MAP as follows?
>>   #define CCMAP_BEGIN_AT_START_OF_MAP \
>>     (PR_BITMASK(CCMAP_BITS_PER_MID_LOG2) << CCMAP_BITS_PER_PAGE_LOG2)
>
>I'm a bit unclear here on this. Wouldn't this make 
>CCMAP_BEGIN_AT_START_OF_MAP have a lot of bits set in the
>UPPER_INDEX at the start (instead of 0 bits set)?

No, because CCMAP_MID_INDEX(c) is ((c)>>CCMAP_BITS_PER_PAGE_LOG2) &
PR_BITMASK(CCMAP_BITS_PER_MID_LOG2) -- note that the mask happens last.  So
overflow into the UPPER bits will be cleared.  Give it a whirl, it should work.

/be

Comment 12

[kill this account]

It starts out okay but never seems to find the end of map. It appears to be
stuck looping over these 3 values:

  aPageStart = 61440
  aPageStart = 64256
  aPageStart = 65280

Comment 13

[kill this account]

I made the other changes and checked in.

If I/we can figure out how to make that last part work I will check in in.

Comment 14

[Brendan Eich [:brendan]]

Never mind, my proposed "tweak" doesn't let mid_index exceed 0xf, but it must
for the loop to do the right thing (as it does with the last attached patch). 
The if else version is good enough, sorry for the goose chase.

/be

Comment 15

[Teruko Kobayashi]

Changed QA contact to [e-mail].