Performace enhancement in CTL code

RESOLVED FIXED in mozilla1.4beta

Status

()

Core
Layout: Text
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: Prabhat Hegde, Assigned: Prabhat Hegde)

Tracking

(Blocks: 1 bug, {intl})

Trunk
mozilla1.4beta
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 121744 [details] [diff] [review]
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.
(Assignee)

Comment 2

15 years ago
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.
(Assignee)

Comment 3

15 years ago
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)
OS: Windows 2000 → Linux
Hardware: Sun → PC

Updated

15 years ago
Blocks: 65896

Comment 4

15 years ago
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.   
Keywords: intl

Comment 5

15 years ago
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? 
Target Milestone: --- → mozilla1.4beta
(Assignee)

Comment 6

15 years ago
Created attachment 123137 [details] [diff] [review]
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.
Attachment #121744 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Blocks: 205260
(Assignee)

Comment 7

15 years ago
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

15 years ago
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.
(Assignee)

Comment 9

15 years ago
Created attachment 123155 [details]
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

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

Comment 11

15 years ago
Created attachment 123577 [details] [diff] [review]
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.

Updated

15 years ago
Blocks: 204286

Comment 12

15 years ago
Created attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])

To move things forward (and make it easier for reviewers), I'm putting up a
combined patch
Attachment #123137 - Attachment is obsolete: true
Attachment #123155 - Attachment is obsolete: true
Attachment #123577 - Attachment is obsolete: true

Comment 13

15 years ago
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 :-)
Attachment #127498 - Flags: superreview?(blizzard)
Attachment #127498 - Flags: review?(katakai)
(Assignee)

Comment 14

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

Comment 15

14 years ago
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...

Updated

14 years ago
Blocks: 166520

Comment 16

14 years ago
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?
(Assignee)

Comment 17

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

Updated

14 years ago
Attachment #127498 - Flags: superreview?(rbs)
Attachment #127498 - Flags: superreview?(blizzard)
Attachment #127498 - Flags: review?(prabhat.hegde)
Attachment #127498 - Flags: review?(katakai)
(Assignee)

Comment 18

14 years ago
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.
Attachment #127498 - Flags: review?(prabhat.hegde) → review+

Comment 19

14 years ago
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.
Attachment #127498 - Flags: superreview?(rbs) → superreview+

Comment 20

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

Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Blocks: 178735

Updated

9 years ago
Component: Layout: CTL → Layout: Text
QA Contact: arthit → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.