Bug 227713 - Improve TextProcessor performance
Summary: Improve TextProcessor performance
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: Framework (show other bugs)
Version: 3.4   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: 3.4 M7   Edit
Assignee: Thomas Watson CLA
QA Contact:
URL:
Whiteboard:
Keywords: performance
Depends on:
Blocks:
 
Reported: 2008-04-18 03:01 EDT by Dani Megert CLA
Modified: 2008-04-23 10:47 EDT (History)
3 users (show)

See Also:


Attachments
Fix (2.95 KB, patch)
2008-04-18 09:55 EDT, Dani Megert CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dani Megert CLA 2008-04-18 03:01:25 EDT
3.4 M6.

The TextProcessor methods always run through that code:

if (str == null || str.length() <= 1 || !isSupportedPlatform || !isBidi)

There are three improvements here:
1. since isSupportedPlatform and isBidi are constants it would make sense to
   fold them into one, e.g. isInstalled (i.e. no need for two constants)
2. boolean tests are cheaper than the string ops, hence test the boolean
   constant first

==> change test in process(String,String) and deprocess(String)

if (isInstalled && str == null || str.length() <= 1)

This saves the string evaluation for all clients that aren't in BIDI mode.


Another (minor) performance improvement would be to test the constant before calling another method:

	public static String process(String text) {
		return process(text, getDefaultDelimiters());
	}

==>
	public static String process(String text) {
                if (!installed)
                    return text;
		return process(text, getDefaultDelimiters());
	}
Comment 1 Danail Nachev CLA 2008-04-18 03:13:03 EDT
I'm totally with you on this, but I wonder does this change actually result in performance improvement?
Comment 2 Dani Megert CLA 2008-04-18 03:16:19 EDT
Yes, if you run millions of string through the processor the gain is already a second or more, mostly due to the string stuff.
Comment 3 Thomas Watson CLA 2008-04-18 09:25:54 EDT
How about a patch? :)
Comment 4 Dani Megert CLA 2008-04-18 09:33:03 EDT
Sure, will attach today (I don't have the project in source otherwise I would have done it).
Comment 5 Dani Megert CLA 2008-04-18 09:55:31 EDT
Created attachment 96599 [details]
Fix
Comment 6 Thomas Watson CLA 2008-04-18 11:24:35 EDT
Thanks, I will take a look at this for M7.
Comment 7 Thomas Watson CLA 2008-04-22 16:46:52 EDT
(In reply to comment #2)
> Yes, if you run millions of string through the processor the gain is already a
> second or more, mostly due to the string stuff.
> 

A second or more, what is the percentage of improvement?  In a typical eclipse scenario I don't think we are talking about millions of strings.  I'll ask Danail's question from comment 1 in a different way.  Is this performance improvement measurable in the startup of a large Eclipse Application?
Comment 8 Dani Megert CLA 2008-04-23 02:25:54 EDT
I can go and attach numbers if you first explain why make the more expensive operation before the cheaper one.
Comment 9 Thomas Watson CLA 2008-04-23 10:22:49 EDT
(In reply to comment #8)
> I can go and attach numbers if you first explain why make the more expensive
> operation before the cheaper one.
> 

I have no good explanation, this code originated in UI and was handed to me to support :)  I actually have no use for the TextProcessor at all and have been complaining about it being in the Framework ever since it was pushed down.  But there is no going back now :(

I think the patch is a good improvement and I was going to release today, I just wanted more justification.
Comment 10 Thomas Watson CLA 2008-04-23 10:26:38 EDT
Patch released.  Thanks for taking the time Daniel.  I do appreciate the help.  BTW, I'm curious what made you look into this bit of code for performace?
Comment 11 Dani Megert CLA 2008-04-23 10:47:50 EDT
>BTW, I'm curious what made you look into this bit of code for performance?
When we started to beef up our code for BIDI I wanted to make sure to have 0 impact on existing clients and that's where TextProcessor code jumped into my eyes.

For some numbers, see comment 2.