203406 - Performace enhancement in CTL code

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Prabhat Hegde]

Current CTL code was developed as hack to support Thai and is quite inefficient
in logic. It appears that there are more users of the code than i initially
suspected. Hence cleaning logic, reducing code-size and memory consumption.

Comment 1

[Prabhat Hegde]

Attachment: Wholesale changes in CTL component to fix performace and inefficient logic

This is a fairly large patch but can't see more efficient way of filing this.

Comment 2

[Prabhat Hegde]

The following problems should also be addressed with this patch:
1. http://bugzilla.mozilla.org/show_bug.cgi?id=122552
2. Fixes warnings caused because nsULE.cpp did not use THREAD_SAFE XPCOM macros.

Comment 3

[Prabhat Hegde]

hi Roland or Masaki, could one of you check this out? If you need the source
files, let me know. (PS: I have run the patch through a tool that checks for bad
format which is the cause of a number of diffs)

Comment 4

[Jungshik Shin]

A bit off-topic here... currently, enable-ctl is only effective
on Unix, right? As it's discussed in bug 203052, Mozilla-Win
doesn't handle complex scripts as well as MS IE and I guess
it's also needed there (unless we decide to use Uniscribe/OTLS
very soon as MSIE does) and perhaps on other platforms (again
unless platform-specific complex script handling such as
Apple's AAT fonts and ATSUI can be utilized by Mozilla easily). 
See also bug 177877, bug 176290 and bug 176315.   

Comment 5

[Jungshik Shin]

Prabhat, can you merge my patch for bug 204286 (attachment 122542 [details] [diff] [review] + the change I
mentioned
in bug 204286 comment #9) with your patch here? I believe both can(should) go in
before 1.4(beta).

Roland and Katakai-san, can we move this on? 

Comment 6

[Prabhat Hegde]

Attachment: Incorporates patch (id=122976)

Fixes warning for comparision between signed and unsigned from patch
(id=122976). Masaki or Roland, please commit. I can't do so myself.

Comment 7

[Prabhat Hegde]

hi Jungshik - I possibly missed your email regarding your change to add isWide.
Why do you require ASCII range to return UCS-2BE? - thanks. After clarification
i will update my Jumbo patch to incorporate your fixes.

Comment 8

[Jungshik Shin]

Hi Prabhat.
Here's my rationale for the change I suggested.

As it stands, what nsUnicodeToSunIndic::Convert() returns is a bit odd 
mixture of US-ASCII and characters in U+09xx and in U+F800(?: PUA). Suppose 
that it's given an input string made up of US-ASCII and Devanagari letters, 
say,'A B U+0935 U+0951 C D'. It returns (assuming no shaping for the sake of 
demonstration) '0x41 0x42 0x09 0x35 0x09 0x51 0x43 0x44'. In this form without 
further processing, I can't pass it to the rendering engine (e.g 
nsFontMetricsXft) which expects the input to be either in *pure* ASCII (8bit 
string : as XDrawString in X11 does) or in *pure* 16bit UCS-2BE (as 
XDrawString16 in X11 does). As I wrote in bug 204286, I can write a simple 
wrapper to turn the output of nsUnicodeToSunIndic::Convert() to 16bit UCS-2BE, 
but that seems to be an uncessary overture  because a simple change I 
suggested in my patch to bug 204286 would make nsUnicodeToSunIndic::Convert() 
return UCS-2BE string that can be directly fed to drawing-routine.

BTW, as you know too well, nsUnicodeToTIS620::Convert() is 'SingleByte' 
converter so that I think adding 'aIsWide' parameter with the default set to 
false makes sense. In the future, I expect nsUnicodeToTamilTTF (bug 204039) 
and nsUnicodeToJamoTTF(bug 176315) to move to intl/ctl to take advantage of 
CTL's selection/cursor movement support in nsTextFrame. They're 
also 'wide'(double-byte) converters. Therefore, adding aIsWide is also to 
prepare for the future :-)

Hope my explanation is clear :-). If not, please let me know and I'll try 
again. Thanks.

Comment 9

[Prabhat Hegde]

Attachment: Attachment in zip format for the unix friendly

Since the patch is too huge, attaching the diff source files. Masaki and
Roland, Can you look at it and putback? (The performance changes have been in
Sun Mozilla tree for sometime now and should hopefully be stopper free).

Comment 10

[timeless]

*** Bug 205260 has been marked as a duplicate of this bug. ***

Comment 11

[Jungshik Shin]

Attachment: an extra patch to keep gcc silent

This has to be applied on top of attachment 123155 [details]. It gets rid of another
signed vs unsigned comparison and adds extra braces around 'sub-arrays' of '2-d
arrays' to make gcc happy.

Comment 12

[Jungshik Shin]

Attachment: a combined patch (attachment 123155 + attachment 123577)

To move things forward (and make it easier for reviewers), I'm putting up a
combined patch

Comment 13

[Jungshik Shin]

Comment on attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])

Asking for r/sr on behalf of prabhat. Hope he doesn't mind :-)

Comment 14

[Prabhat Hegde]

Definitely not! Thanks for the patch and help in moving the bug along.
prabhat.

Comment 15

[Jungshik Shin]

Katakai-san, can you review the patch? Prabhat wrote that it'd been a part of
Sun's internal build for the last half year or so...

Comment 16

[Andreas Lange]

Bug 178735 regards cursor movement beeing broken in all textareas (textarea,
composer, mail etc) if compiling with --enable-ctl. It has been that way since
1.4. Does this huge patch solve that one too?

Comment 17

[Prabhat Hegde]

Possibly Louie Zhao can also review this. The perf enhancement part has been in
Sun's Mozilla codebase since long while - Louie?
prabhat

Comment 18

[Prabhat Hegde]

Comment on attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])

Looks good to me. Original patch has been working uneventfully in Sun's Mozilla
for the past year.

Comment 19

[rbs]

Comment on attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])

rs=rbs based on the year long testing at Sun.

Comment 20

[Jungshik Shin]

At very long last, fix checked into the trunk
thanks all.