OFBiz
  1. OFBiz
  2. OFBIZ-4268

ResourceBundleMapWrapper memory usage improvement

    Details

      Description

      Hello,
      We are working on performance improvements while load testing the application, and I noticed that ResourceBundleMapWrapper uses arounbd 140 Ko per Serving thread, in our configuration with around 1000 thread, this is a big memory impact.
      I started investigating the class and code seems strange to me, in fact I don't understand why InternalRbmWrapper constructor copies creates a Map by copying content of ResourceBundle.
      Why not only have a reference and delegate calls to the ResourceBundle ?
      Furthermore since put methods throw RuntimeException, I really don't see any case where the Map will change.
      I made a test removing this code and delegating to resourceBundle and it seems OK.

      If someone knows why it was made like that I would really be interested, if there is no reason I will submit a patch

      Thank you
      Regards
      Philippe
      http://www.ubik-ingenierie.com

        Activity

        Hide
        Adrian Crum added a comment -

        Phillipe,

        I believe the main purpose of ResourceBundleMapWrapper is to provide a read-only Map interface for ResourceBundle objects. It also implements a MapStack - so multiple bundles can be accessed from a single object. The most common use of this class is the "uiLabelMap" you see used throughout the project.

        A while ago I tried to clean up that class (rev 618892) but I had to revert my changes because they introduced a lot of bugs. So, while you're working on this be aware of unintended side effects.

        Show
        Adrian Crum added a comment - Phillipe, I believe the main purpose of ResourceBundleMapWrapper is to provide a read-only Map interface for ResourceBundle objects. It also implements a MapStack - so multiple bundles can be accessed from a single object. The most common use of this class is the "uiLabelMap" you see used throughout the project. A while ago I tried to clean up that class (rev 618892) but I had to revert my changes because they introduced a lot of bugs. So, while you're working on this be aware of unintended side effects.
        Hide
        Philippe Mouawad added a comment -

        Hello,
        Thank you for your very rapid response.
        I propose to implement the following:

        • I will just lazy build the Map once it is requested but till then I will use ResourceBundle, so this will not change the behaviour, It will make us gain CPU + Memory when methods not using map will be accessed.

        My first tests seem OK but we plan a full UTA so if I submit this patch be confident It will work.

        Regards
        Philippe
        http://www.ubik-ingenierie.com

        Show
        Philippe Mouawad added a comment - Hello, Thank you for your very rapid response. I propose to implement the following: I will just lazy build the Map once it is requested but till then I will use ResourceBundle, so this will not change the behaviour, It will make us gain CPU + Memory when methods not using map will be accessed. My first tests seem OK but we plan a full UTA so if I submit this patch be confident It will work. Regards Philippe http://www.ubik-ingenierie.com
        Hide
        Philippe Mouawad added a comment -

        Patch that creates the Map only when it is required

        Show
        Philippe Mouawad added a comment - Patch that creates the Map only when it is required
        Hide
        Philippe Mouawad added a comment -

        Hello,
        Here is the patch, our load tests were concluding, both memory usage and CPU where reduced.

        Full UAT will occur within next 2 weeks, I will comment when they are ended.
        Regards
        Philippe
        http://www.ubik-ingenierie.com

        Show
        Philippe Mouawad added a comment - Hello, Here is the patch, our load tests were concluding, both memory usage and CPU where reduced. Full UAT will occur within next 2 weeks, I will comment when they are ended. Regards Philippe http://www.ubik-ingenierie.com
        Hide
        Philippe Mouawad added a comment -

        Hello,
        UAT have been successful, patch is OK and has been put in production

        Regards
        Philippe
        http://www.ubik-ingenierie.com

        Show
        Philippe Mouawad added a comment - Hello, UAT have been successful, patch is OK and has been put in production Regards Philippe http://www.ubik-ingenierie.com
        Hide
        Jacques Le Roux added a comment -

        Hi Philippe,

        After reviewing (not a simple one), I have done some tests locally (without cluster) and so far I saw no problems. However I never crossed any uses of createMapWhenNeeded(). Have you a scenario to demonstrate its use (steps, URL)?

        Last but not least: you have tabs in your patch. When I change to 4 spaces it breaks the formatting. Please provide a patch without tabs, thanks

        Show
        Jacques Le Roux added a comment - Hi Philippe, After reviewing (not a simple one), I have done some tests locally ( without cluster ) and so far I saw no problems. However I never crossed any uses of createMapWhenNeeded(). Have you a scenario to demonstrate its use (steps, URL)? Last but not least: you have tabs in your patch. When I change to 4 spaces it breaks the formatting. Please provide a patch without tabs, thanks
        Hide
        Philippe Mouawad added a comment -

        Hello Jacques,
        In fact I don't see any case where createMapWhenNeeded() will be used, but if you look at Adrian comment, I was afraid of introducing a regression so I kept this behaviour but made it lazy so building the map will occur only when it is required to (so will never happen I think)

        Regards
        Philippe
        http://www.ubik-ingenierie.com

        Show
        Philippe Mouawad added a comment - Hello Jacques, In fact I don't see any case where createMapWhenNeeded() will be used, but if you look at Adrian comment, I was afraid of introducing a regression so I kept this behaviour but made it lazy so building the map will occur only when it is required to (so will never happen I think) Regards Philippe http://www.ubik-ingenierie.com
        Hide
        Jacques Le Roux added a comment -

        Arg, closed by error, always this Cancel button :/

        Show
        Jacques Le Roux added a comment - Arg, closed by error, always this Cancel button :/
        Hide
        Jacques Le Roux added a comment - - edited

        Thanks Philippe,

        Your patch is in trunk at r1100138

        Please remember to use spaces in your patches and not tabs (formatting and comparaison issues).

        I finally removed a duplicated snippet (no harms just useless)

        Show
        Jacques Le Roux added a comment - - edited Thanks Philippe, Your patch is in trunk at r1100138 Please remember to use spaces in your patches and not tabs (formatting and comparaison issues). I finally removed a duplicated snippet (no harms just useless)

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Philippe Mouawad
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development