Closed Bug 203406 Opened 22 years ago Closed 21 years ago

Performace enhancement in CTL code

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.4beta

People

(Reporter: prabhat.hegde, Assigned: prabhat.hegde)

References

(Blocks 1 open bug)

Details

(Keywords: intl)

Attachments

(1 file, 4 obsolete files)

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.
This is a fairly large patch but can't see more efficient way of filing this.
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.
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
Blocks: thai
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
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
Attached patch Incorporates patch (id=122976) (obsolete) (deleted) — Splinter Review
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
Blocks: 205260
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.
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.
Attached file Attachment in zip format for the unix friendly (obsolete) (deleted) —
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).
*** Bug 205260 has been marked as a duplicate of this bug. ***
Attached patch an extra patch to keep gcc silent (obsolete) (deleted) — Splinter Review
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.
Blocks: 204286
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 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)
Definitely not! Thanks for the patch and help in moving the bug along. prabhat.
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...
Blocks: 166520
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?
Possibly Louie Zhao can also review this. The perf enhancement part has been in Sun's Mozilla codebase since long while - Louie? prabhat
Attachment #127498 - Flags: superreview?(rbs)
Attachment #127498 - Flags: superreview?(blizzard)
Attachment #127498 - Flags: review?(prabhat.hegde)
Attachment #127498 - Flags: review?(katakai)
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 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+
At very long last, fix checked into the trunk thanks all.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Blocks: 178735
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.

Attachment

General

Creator:
Created:
Updated:
Size: