[jira] Created: (OFBIZ-1485) UtilProperties - The Next Generation

[Nicolas Malin (Jira)]

UtilProperties - The Next Generation
------------------------------------

                 Key: OFBIZ-1485
                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
             Project: OFBiz
          Issue Type: Improvement
          Components: framework
            Reporter: Adrian Crum
            Priority: Minor


Improve the UtilProperties class. Details in comments.


--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

     [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Adrian Crum updated OFBIZ-1485:
-------------------------------

    Attachment: props_tng.patch

While I was working on adding the XML properties file capability to UtilProperties, I noticed a few things about the UtilProperties class that bothered me. After the initial XML properties support was committed (rev 599356) I went back to the UtilProperties class to work on those issues.

#1 The caching algorithm in getInternalRbmWrapper(...) (line 493) - could potentially store multiple copies of the same ResourceBundle in the cache.
#2 The getProperties(String resource, Locale locale) method wasn't fully implemented (see notes at line 546).

Issue #1 was easy to fix - I just used the ResourceBundle.getLocale() method to do a second cache lookup, around line 506. If one was found, I cached the new key with the existing bundle. Now multiple locales could share a single ResourceBundle instance. There was still the potential to have multiple copies of the same ResourceBundle though. I'll get back to that later.

The solution to issue #2 was obvious - UtilProperties needed a properties file location resolver. I already had one working in the XmlResourceBundle inner class, so I just moved that code from the inner class to the outer class. I did the same for the XML properties file to Map conversion code.

At this point, everything was working well - but something else started to bother me. Why does UtilProperties have three caches? They all contain basically the same thing. Technically, UtilProperties only needs two - one for non-locale-specific properties, and one for locale-specific properties. I changed the code a bit so that the ResourceBundle cache wasn't needed - I wrote a method that wraps a XmlResourceBundle (renamed to UtilResourceBundle) instance around an existing cached properties object. Methods that need a ResourceBundle call the wrapper method. That eliminated one cache. That also solved the issue that bugged me about the multiple same ResourceBundle instances in the cache.

Now that the code was reduced down to two caches of FlexibleProperties objects, I started to look at the FlexibleProperties class to see if there was a way to convert it to a Javalution FastMap. So I traced through the UtilProperties-to-FlexibleProperties code. I determined that the FlexibleProperties object contributes nothing to the UtilProperties class. I converted all references to FlexibleProperties to plain Maps, and used the Javalution FastMap for new instances.

The final step was to rewrite bits of code so that UtilProperties would pass around Maps internally, and only use the Properties class when one is requested.

The attached patch is the result. I have used it for about a week - it seems to work well. The performance gains, memory savings, etc will have to be determined by real-world installations.

Comments are welcome.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548128 ]

Adrian Crum commented on OFBIZ-1485:
------------------------------------

If/when this patch is committed, the next step would be to take a look at the ResourceBundleMapWrapper class - it might not be needed. Here is the result of new UtilProperties class:

Get a properties Map
  Convert it to a ResourceBundle
    Use ResourceBundleMapWrapper to convert ResourceBundle back to a Map

Seems clunky to me. Peripheral code could convert a UtilProperties Map to a Mapstack and do whatever magic it wants to do with it.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548122 ]

[hidden email] edited comment on OFBIZ-1485 at 12/3/07 9:53 PM:
-------------------------------------------------------------

While I was working on adding the XML properties file capability to UtilProperties, I noticed a few things about the UtilProperties class that bothered me. After the initial XML properties support was committed (rev 599356) I went back to the UtilProperties class to work on those issues.

#1 The caching algorithm in getInternalRbmWrapper(...) (line 493) - could potentially store multiple copies of the same ResourceBundle in the cache.
#2 The getProperties(String resource, Locale locale) method wasn't fully implemented (see notes at line 546).

Issue #1 was easy to fix - I just used the ResourceBundle.getLocale() method to do a second cache lookup, around line 506. If one was found, I cached the new key with the existing bundle. Now multiple locales could share a single ResourceBundle instance. There was still the potential to have multiple copies of the same ResourceBundle though. I'll get back to that later.

The solution to issue #2 was obvious - UtilProperties needed a properties file location resolver. I already had one working in the XmlResourceBundle inner class, so I just moved that code from the inner class to the outer class. I did the same for the XML properties file to Map conversion code.

At this point, everything was working well - but something else started to bother me. Why does UtilProperties have three caches? They all contain basically the same thing. Technically, UtilProperties only needs two - one for non-locale-specific properties, and one for locale-specific properties. I changed the code a bit so that the ResourceBundle cache wasn't needed - I wrote a method that wraps a XmlResourceBundle (renamed to UtilResourceBundle) instance around an existing cached properties object. Methods that need a ResourceBundle call the wrapper method. That eliminated one cache. That also solved the issue that bugged me about the multiple same ResourceBundle instances in the cache.

Now that the code was reduced down to two caches of FlexibleProperties objects, I started to look at the FlexibleProperties class to see if there was a way to convert it to a Javalution FastMap. So, I traced through the UtilProperties-to-FlexibleProperties code and I determined that the FlexibleProperties class contributes nothing to the UtilProperties class. So, I converted all references to FlexibleProperties to plain Maps, and used the Javalution FastMap for new instances.

The final step was to rewrite bits of code so that UtilProperties would pass around Maps internally, and only use the Properties class when one is requested.

The attached patch is the result. I have used it for about a week - it seems to work well. The performance gains, memory savings, etc will have to be determined by real-world installations.

Comments are welcome.


      was (Author: [hidden email]):
    While I was working on adding the XML properties file capability to UtilProperties, I noticed a few things about the UtilProperties class that bothered me. After the initial XML properties support was committed (rev 599356) I went back to the UtilProperties class to work on those issues.

#1 The caching algorithm in getInternalRbmWrapper(...) (line 493) - could potentially store multiple copies of the same ResourceBundle in the cache.
#2 The getProperties(String resource, Locale locale) method wasn't fully implemented (see notes at line 546).

Issue #1 was easy to fix - I just used the ResourceBundle.getLocale() method to do a second cache lookup, around line 506. If one was found, I cached the new key with the existing bundle. Now multiple locales could share a single ResourceBundle instance. There was still the potential to have multiple copies of the same ResourceBundle though. I'll get back to that later.

The solution to issue #2 was obvious - UtilProperties needed a properties file location resolver. I already had one working in the XmlResourceBundle inner class, so I just moved that code from the inner class to the outer class. I did the same for the XML properties file to Map conversion code.

At this point, everything was working well - but something else started to bother me. Why does UtilProperties have three caches? They all contain basically the same thing. Technically, UtilProperties only needs two - one for non-locale-specific properties, and one for locale-specific properties. I changed the code a bit so that the ResourceBundle cache wasn't needed - I wrote a method that wraps a XmlResourceBundle (renamed to UtilResourceBundle) instance around an existing cached properties object. Methods that need a ResourceBundle call the wrapper method. That eliminated one cache. That also solved the issue that bugged me about the multiple same ResourceBundle instances in the cache.

Now that the code was reduced down to two caches of FlexibleProperties objects, I started to look at the FlexibleProperties class to see if there was a way to convert it to a Javalution FastMap. So I traced through the UtilProperties-to-FlexibleProperties code. I determined that the FlexibleProperties object contributes nothing to the UtilProperties class. I converted all references to FlexibleProperties to plain Maps, and used the Javalution FastMap for new instances.

The final step was to rewrite bits of code so that UtilProperties would pass around Maps internally, and only use the Properties class when one is requested.

The attached patch is the result. I have used it for about a week - it seems to work well. The performance gains, memory savings, etc will have to be determined by real-world installations.

Comments are welcome.

 

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548170 ]

Jonathon Wong commented on OFBIZ-1485:
--------------------------------------

> Now that the code was reduced down to two caches of FlexibleProperties
> objects, I started to look at the FlexibleProperties class to see if there
> was a way to convert it to a Javalution FastMap. So, I traced through the
> UtilProperties-to-FlexibleProperties code and I determined that the
> FlexibleProperties class contributes nothing to the UtilProperties class. So,
> I converted all references to FlexibleProperties to plain Maps, and used the
> Javalution FastMap for new instances.

A few points here.

FlexibleProperties does contribute something. Look at the methods in there.

A Map is different from a Properties object, less restrictive. In this case, less restrictive is not good. In fact, Properties wraps around Hashtable mainly to prevent non-strings from being inserted into the underlying Map.

You called for suggestions to use the new powerful FastMap while avoiding breaking legacy codes that may expect methods getProperties(...) to return Properties. I suggested that FlexibleProperties class internally deals with a FastMap object, while also overriding all the methods in the Properties class. That way, you get absolutely zero need for change propagation. All legacy codes continue to work; FastMap is now used. If so inclined, you may want to create a new Properties subclass called "FastProperties".

I don't think we should make methods getProperties(...) return anything other than Properties. As mentioned above, a Map is less restrictive and does not confine entered values to "just properties" (which are usually "just strings").

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548282 ]

Adrian Crum commented on OFBIZ-1485:
------------------------------------

Jonathon,

Thank you for taking the time to review the code. If you look closely at the existing code, FlexibleProperties contributes nothing to UtilProperties. It may have additional methods, but they aren't being used anywhere. UtilProperties treats FlexibleProperties as a generic Properties object.

I understand a Map is less restrictive than a Properties object - that's why the methods that returned Properties objects still do so. In addition, we have new methods that return Maps - and those Maps use Generics to enforce the Key is String and Value is String constraints.

Building a custom Properties class around a FastMap is an interesting idea, but I don't think it's necessary. If a piece of code MUST have a Properties object, one can be constructed from a FastMap by using the Properties.putAll(...) method.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548530 ]

Jonathon Wong commented on OFBIZ-1485:
--------------------------------------

> UtilProperties treats FlexibleProperties as a generic Properties object.

That's the concept of abstraction. Introduce an additional layer (FlexibleProperties) between UtilProperties and Properties. Idea is to decouple UtilProperties from Properties, so that the underlying implementation of Properties can be swapped easily. Like now, for example. You want a FastMap, just change FlexibleProperties. UtilProperties don't need change, nor do any codes that use UtilProperties. Abstraction. Decoupling. Pluggable architecture.

What happens when you want to change FastMap to UltraMap in future? You'd need to change UtilProperties, and possibly even codes that use UtilProperties. Maybe UltraMap has some new ultra interfaces that FastMap doesn't have.

Don't teach the driver to drive differently. Teach the car! Whether the car has an old fuel injection system (FastMap) or a new nano-spray injection system (UltraMap), the driver still steps on the gas pedal the same as always. The gas pedal could be your abstraction layer, the FlexibleProperties.

Even if FlexibleProperties add little value now, it should still be retained as an insulation layer between UtilProperties and Properties. It's not always a step forward to reduce the number of classes, especially in object-oriented programming where abstraction is useful.

> those Maps use Generics to enforce the Key is String and Value is String
> constraints

"Generics" is a class? Interface?

The fact that you are making Maps perform extra acrobatics for such enforcement already tells you that a "specialization" is needed. Rather than making a very special Map that acts like Properties, isn't it more logical to make a FastProperties?

> If a piece of code MUST have a Properties object, one can be constructed from
> a FastMap by using the Properties.putAll(...) method.

Then you're teaching the driver to steer different. You wanted legacy codes (or existing users of those methods) to avoid change, right? Or not? Don't we want to benefit more (with fast FastMap) and work less (less change propagation)?

If a piece of code does not have a Properties object (which is what you're suggesting now), there is a chance that a non-string "property" will get inserted into the Map object. Somewhere down the line, something will fail, logics get compounded, and problems become convoluted.

Consider the "fail-fast" strategy that reduces programming errors and increases deterministic program flow.

The OFBiz framework is very very well-done by solid OO programmers (delegators, pluggable view handlers, wow!). Continue the tradition? :)

Is the solution I suggested very problematic? I thought it got us all the benefit (with FastMap) and none of the hassle (users of UtilProperties need zero change)? What is the objective here, actually?

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

     [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Adrian Crum updated OFBIZ-1485:
-------------------------------

    Attachment: props_tng.patch

Updated patch. Much improved code.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch, props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12549068 ]

Adrian Crum commented on OFBIZ-1485:
------------------------------------

This isn't quite ready for committing. I ran some performance tests last night and there is no speed difference between a Javalution FastMap and a java.utilProperties object. So, I'll revert the non-locale-specific properties code back to FlexibleProperties.

I also want to build out the XML code a little more so that it supports the Java XML properties file format in addition to our custom format. I also need to figure out why the XML properties file parsing is running four times slower than a regular properties file.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch, props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12549298 ]

Jonathon Wong commented on OFBIZ-1485:
--------------------------------------

Performance gains for FastMap happens when dealing with huge maps. See documentation for FastMap, they have performance benchmarks there.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch, props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

     [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Adrian Crum updated OFBIZ-1485:
-------------------------------

    Attachment: props_tng.patch

Updated patch. I think this is the final version.

Most of the code below this line:

// ========= Locale & Resource Based Methods ==========

has been refactored.

To summarize:
1. Improved XML file support
2. Improved ResourceBundle caching
3. New method for resolving properties file locations
4. Groundwork laid for Java 6 improved ResourceBundle class

It would be cool if we could get a few people to test it. The main things to look for:
1. UI labels display properly regardless of properties file format
2. UI labels "fall back" to a less specific locale if a particular label doesn't exist in the user's locale

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch, props_tng.patch, props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

     [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Adrian Crum updated OFBIZ-1485:
-------------------------------

    Attachment: props_tng.patch

Updated patch to latest svn.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch, props_tng.patch, props_tng.patch, props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[Nicolas Malin (Jira)]

     [ https://issues.apache.org/jira/browse/OFBIZ-1485?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Adrian Crum closed OFBIZ-1485.
------------------------------

    Resolution: Fixed

Some of the improvements mentioned in this issue have been committed to the trunk - rev 607574.

> UtilProperties - The Next Generation
> ------------------------------------
>
>                 Key: OFBIZ-1485
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-1485
>             Project: OFBiz
>          Issue Type: Improvement
>          Components: framework
>            Reporter: Adrian Crum
>            Priority: Minor
>         Attachments: props_tng.patch, props_tng.patch, props_tng.patch, props_tng.patch
>
>
> Improve the UtilProperties class. Details in comments.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.