Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory footprint optimization and performance improvement #3118

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

sivakiran4u1
Copy link

@sivakiran4u1 sivakiran4u1 commented Mar 21, 2024

Team,

We observed high memory usage of configuration object on heavy mybatis applications. We observed that, we are repeatedly parsing the same XNodes and creating SQL nodes, instead of reusing the previously created SQLNode for the Xnode.
These changes holds the sqlNodeMap which consists of Xnode String(query String) as key, and respective SQLNode as value. So that, whenver we came across the same Xnode reference, we will use the previously created object from Map. This reduces the overall SQLNode Objects, and memory foot print of Configuration object.

These changes are already tested and validated in our project for more 12months.

With this change, My application memory dropped from 1.2GB to 350MB.

sivakiran4u and others added 15 commits November 4, 2022 10:03
…/dc/dc-mybatis into release/3.5.11.1

# Conflicts:
#	pom.xml
#	src/main/java/org/apache/ibatis/parsing/XNode.java
#	src/main/java/org/apache/ibatis/scripting/xmltags/XMLScriptBuilder.java
…to inampuds/3.5.15

* '3.5.15' of https://github.com/sivakiran4u1/mybatis-3: (382 commits)
  Update license year
  fix: DefaultResultSetHandler memory waste mybatis#3113
  chore(deps): update dependency org.postgresql:postgresql to v42.7.3
  chore(deps): update log4j2 monorepo to v2.23.1
  chore(deps): update testcontainers-java monorepo to v1.19.7
  chore(deps): update dependency ch.qos.logback:logback-classic to v1.5.3
  chore(deps): update dependency ch.qos.logback:logback-classic to v1.5.2
  chore(deps): update mockito monorepo to v5.11.0
  chore(deps): update dependency ch.qos.logback:logback-classic to v1.5.1
  chore(deps): update dependency org.testcontainers:mysql to v1.19.6
  chore(deps): update log4j2 monorepo to v2.23.0
  fix(deps): update dependency com.microsoft.sqlserver:mssql-jdbc to v12.6.1.jre11
  chore(deps): update dependency org.postgresql:postgresql to v42.7.2
  chore(deps): update dependency ch.qos.logback:logback-classic to v1.5.0
  chore(deps): update byte-buddy.version to v1.14.12
  [git] Update git ignore
  chore(deps): update actions/setup-java action to v4
  chore(deps): update jamesives/github-pages-deploy-action action to v4.5.0
  [GHA] Update sonar.login to sonar.token
  [pom] Remove topSiteURL as defined in parent now
  ...

# Conflicts:
#	pom.xml
#	src/main/java/org/apache/ibatis/mapping/BoundSql.java
#	src/main/java/org/apache/ibatis/scripting/xmltags/XMLScriptBuilder.java
#	src/main/java/org/apache/ibatis/session/Configuration.java
@harawata
Copy link
Member

This reduces memory usage only if there are a lot of identical SQL texts in mappers, am I right?
It is good that it works well for your application, but for most application, it may actually increase memory usage, I think.
We cannot optimize the core code for extreme usage scenario.

It's still helpful for us to know the usage and the problem, so I appreciate you taking the time to submit this PR.
I'll keep it in mind when designing future version.

@sivakiran4u1
Copy link
Author

As per our observations, Even in our small size applications... these changes havent causes any increase on memory footprint. This change also improving performance while building configuration object, by reusing existing Xnodes, instead of parsing whole hierarchy.

Note: To minimize the impact this new map on memory.. we are using hasy key of the string content as key. This helped us to keep the memory addition very minimal.

@hazendaz
Copy link
Member

hazendaz commented Mar 24, 2024

@sivakiran4u1 Please update the title of this PR to have a better description than the version number.

@sivakiran4u1 sivakiran4u1 changed the title 3.5.15 Memory footprint optimization and performance improvement Mar 24, 2024
@youngledo
Copy link

May I ask what the conclusion is?

@harawata
Copy link
Member

Hello @youngledo ,

I was going to close this (see my earlier comment for why).

I also noticed that it contains at least one backward incompatible change.
There was no test covering that usage, so I added a new one to avoid same/similar regression.
(Please do not try to fix the breakage. I don't want to waste anyone's time.)

I'll keep this open in case @hazendaz has a different opinion than mine.

@hazendaz
Copy link
Member

I'm not a against adding improvements but there are problem better code to be written in here. Maybe let this sit a while so we can get our goal of getting to java 17 and start improving underlying code in general. Then see if its still needed. @youngledo What java version do you use where you had the improvements?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants