Fix XPathParts()

Description

We have a cache for XPathParts which is faster than using the constructor.

I proposed refactoring to change

x = new XPathParts();

to

x = XPathParts.getFrozenInstance(path).cloneAsThawed();

Now, it is even faster to remove the .cloneAsThawed(), but we can only do that if x is not going to be changed, which requires code inspection.

xpath

None

locale

None

Activity

Show:
Mark Davis
December 2, 2019, 8:18 PM

Changing to John for reviewer, since he did the github review.

Thomas Bishop
November 27, 2019, 4:50 PM

The PR was accepted and merged.

Thomas Bishop
November 6, 2019, 8:28 PM

The XPathParts parameter to getUndistinguishingElementsFor is unused and should be removed.

There is one call to “new XPathParts.Comments()” which is unrelated to this issue.

There are four calls (all in CLDRFile.write) to “new XPathParts(defaultSuppressionMap)”, which require special treatment; see Mark’s comments above.

The usage of CLDRFile.nondraftParts is strange: an empty XPathParts object is allocated, but then never accessed except for a block with “synchronized(nondraftParts)”. Seemingly there is no reason for this object to be an XPathParts; it could just as well be some other kind of object.

In DtdData.XPathPartsSet, GenerateDecompCollationRules.main, and GenerateLanguageData.main, the XPathParts object is used in an exceptional way. It is never loaded with a complete path. Instead it is loaded with individual elements with addElement, so getFrozenInstance isn’t a possibility.

XMLModify.main only uses “new XPathParts()” to create an empty “last” (previous) object for the first time writeDifference is called. An alternative, maybe slightly more efficient, would be to revise writeDifference to accept null for this purpose.

Two calls to “new XPathParts()” are internal to XPathParts.java.

Two special-purpose utilities calling “new XPathParts()”, JsonConverter.main and GenerateCoverageLevels.main, are both currently broken. They should be deleted or fixed.

That’s a full account of all usages of “new XPathParts”.

Thomas Bishop
November 6, 2019, 7:04 PM

There are 36 occurrences of “new XPathParts”, including some that are commented out or superfluous (returned object is never used). Removing those brings the total down to 19 occurrences.

Mark Davis
October 17, 2019, 8:53 PM

Here is an idea for refactoring, using Eclipse refactoring

  1. refactor the constructors to use a new getFrozenInstance2 static methods

  2. the new constructors called by getFrozenInstance2 should be private.

  3. alter contents of getFrozenInstance2 to call getFrozenInstance

  4. inline the new factory methods. That will change the call sites to call getFrozenInstance

Change the “XPathParts.getFrozenInstance().set(“ to be “XPathParts.getFrozenInstance(“

There will be some .set methods remaining that will need inspection.

  1. If the object needs to be modifiable, change the factoryMethod creating it to be XPathParts.getInstance.

  2. Run the tests (& console check) to find any remaining places where getFrozenInstance needs to be changed to getInstance

 

Fixed

Priority

medium

Assignee

Thomas Bishop

Reporter

Mark Davis

Reviewer

John Emmons

Labels

Components

Fix versions

Phase

dsub