[Unicode]   Common Locale Data Repository : Bug Tracking Home | Site Map | Search
 
Modify

CLDR Ticket #6995(closed: fixed)

Opened 5 years ago

Last modified 5 years ago

SimpleFactory.HandleMake: Merge two Maps into one, introduce key object

Reported by: ribnitz Owned by: ribnitz
Component: unknown Data Locale:
Phase: Review: mark
Weeks: Data Xpath:
Xref:

ticket:7108

Description

The current implementation of SimpleFactory.handleMake uses two tables (one for resolved entries, the other for the other entries. At the beginning, one of the two maps is chosen, based on the resolved parameter passed into the method.

I propose to merge the two tables into one, and to replace the key used for indexing: Instead of using a String (the locale name), use a compound key (which uses the locale, the resolved status, the minimal draft status and the parent directory). Tests done show that the runtime of the TestAll fixture improves by about half a minute, when the size of the LRU map that is used to cache the data is increased from currently 15 to 210 entries.

Attachments

Change History

comment:1 Changed 5 years ago by emmons

  • Owner changed from anybody to ribnitz
  • Milestone changed from UNSCH to 25final

comment:2 Changed 5 years ago by ribnitz

  • Status changed from new to reviewing
  • Review set to mark

Current settings are: no caching of Factory instances; use 75 entries to cache locales; uses Guava

comment:3 Changed 5 years ago by mark

  • Status changed from reviewing to accepted

SimpleFactory.java

I think this needs some more detailed review. For example, we are doing a file-system tree walk on every access, which is not going to be particularly speedy. The code seems more convoluted than necessary, so let's sit down and walk through it.

Minor:

final int prime = 31;

This should be a constant (static final)

97 if (obj == null) {
98 return false;
99 }

we know we'll never compare against null, and better to just catch the exception (see below).

100 if (getClass() != obj.getClass()) {
101 return false;
102 }

While this is in theory better, in a private class we know the usage. Faster to just cast and catch the exception.

104 if (directory == null) {
105 if (other.directory != null) {
106 return false;
107 }

Can the directory or locale be null? If so, as a key, would be better to set to the empty string, to make equals faster. Also, create and cache the hash code, and compare that first.

Same goes for other new classes, like SimpleFactoryLookupKey

comment:4 Changed 5 years ago by emmons

  • Status changed from accepted to reviewing

comment:5 Changed 5 years ago by mark

  • Xref set to 7108

Additional work needs to be done, so split the bug, with the remaining work in ticket:7108

comment:6 Changed 5 years ago by mark

  • Status changed from reviewing to closed
  • Resolution set to fixed
View

Add a comment

Modify Ticket

Action
as closed
Next status will be 'new'
Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.