BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる #40

morioka-homare · 2025-02-06T02:36:03Z

不具合内容

BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる

修正内容

BeanUtil.copy メソッドを修正
- 全エントリでループしている箇所を、まずネストしていないプロパティだけを処理し、ネストしているエントリは変数に保持しておく。この時にネストのルート部分をキー、それに紐づくエントリーのリストをバリューとしてMap形式で保存する。
- 上記のMapでキーでループしつつ、さらにバリューのリストでループして処理を行うようにする。

テスト

以下のテストコードを追加
- 移送先beanのプロパティの種類、ネストの数に応じた各ケース
  - プロパティの種類は、プリミティブ型、クラス、クラスの配列、クラスのリスト、レコード、レコードの配列、レコードのリスト
- 大量のオブジェクトがある場合のケース
- 今回修正したprivateメソッドを使用しているBeanUtil.setPropertyメソッドの動作確認

tisuchida · 2025-02-07T06:17:34Z

src/main/java/nablarch/core/beans/BeanUtil.java

+
+            PropertyExpression expression = new PropertyExpression(entry.getKey());
+
+            if (expression.isSimpleProperty() && expression.isNode()) {


実装の話ですが、
まずexpression.isNode()で分岐させて、その中で expression.isSimpleProperty() かどうかを見たほうがすっきりするかなと
（261行目でも expression.isNode() を参照しているので）

全体的に構成を修正しました。

tisuchida · 2025-02-07T06:18:34Z

src/main/java/nablarch/core/beans/BeanUtil.java

+        Object destObject;
+
+        Class<?> propertyType = getPropertyType(bean.getClass(), propertyName);
+        if (propertyType.isArray()) {


この部分、 setArrayProperty() と同様にメソッドに切り出した方が良いかなと思いました。

こちらと上の方とで、同じような抽象度になるはずなので

全体的に構成を修正しました。

tisuchida · 2025-02-07T06:20:24Z

src/main/java/nablarch/core/beans/BeanUtil.java

+                    Array.set(propertyObject, propertyExpression.getListIndex(), destObject);
+                }
+            }
+        } else if (List.class.isAssignableFrom(propertyType)) {


こちらも同様に、setListProperty()のようなメソッドになりそうですね

全体的に構成を修正しました。

tisuchida · 2025-02-07T07:29:30Z

src/main/java/nablarch/core/beans/BeanUtil.java

+        for (Map.Entry<String, Map<String, Object>> entry : groupMap.entrySet()) {
+            try {
+                PropertyExpression nestedPropertyExpression = new PropertyExpression(entry.getKey());
+                setProperty(destObject, nestedPropertyExpression, entry.getValue(), copyOptions.reduce(nestedPropertyExpression.getRoot()));


439行目や417行目でConversionUtil.convertを使っていますが、こちらでは使わなくて大丈夫でしょうか。

既存の処理と同様にしています

tisuchida · 2025-02-07T07:56:13Z

src/main/java/nablarch/core/beans/BeanUtil.java

                }
            } catch (BeansException bex) {
                LOGGER.logDebug(
                        "An error occurred while writing to the property :" + entry.getKey());
            }
        }
+
+        for (Map.Entry<String, Map<String, Object>> entry : groupedMap.entrySet()) {


この部分、setProperty でも同じような処理が出てきたと思いますが、まとめるのは難しいでしょうか？

これはほぼ同じことを思います。

Mapの組み方は2種類、その後のループの内容が若干異なるのが差分のようなので、集約できそうに見えます。

全体的に構成を修正しました。

tisuchida · 2025-02-07T08:11:06Z

src/main/java/nablarch/core/beans/BeanUtil.java

@@ -857,43 +959,100 @@ static Map<String, Object> getReducedMap(String rootProperty, Map<String, ?> map
                } else if (expression.isListOrArray()) {
                    Class<?> propertyType = getPropertyType(beanClass, expression.getListPropertyName());
                    if (propertyType.isArray()) {
-                        setArrayPropertyToMap(beanClass, expression, propertyMap, map, copyOptions);
+                        if (expression.isNode()) {


ここも、 expression.isNode() の判定は外側で実施するのが良い気がします。

全体的に構成を修正しました。

charon-r13b · 2025-02-12T08:34:30Z

src/main/java/nablarch/core/beans/BeanUtil.java

+        Map<String, Object> nodeMap = new HashMap<>();
+        Map<String, Object> nestedMap = new HashMap<>();
+
+        for(Map.Entry<String, ?> entry : map.entrySet()) {


処理自体の話ではないですが、時々for(Map.Entryのように空白が抜けたりしているので、修正箇所についてはフォーマッタをかけておいてください。

charon-r13b · 2025-02-12T08:52:59Z

src/main/java/nablarch/core/beans/BeanUtil.java

+        List<Map<String,Object>> separatedList = getSeparatedlist(map);
+
+        for (int i = 0; i< 2; i++) {
+            boolean isNode = i == 0;
+            Map<String, Object> srcMap = separatedList.get(i);


第三者から見ると「iが0だとどうしてノードなのか？」と不思議に思います。

引数のmapをそのまま使うのではなく、getSeparatedlistを必要とする理由は以降の処理で不要なループを増やさないためですよね？

メソッド名はgetSeparatedlistではなくgroupByNodeTypeなどにしてはどうでしょうか。

また返すのをListではなくMapにして、Mapのキーで意味を識別させた方がよいような気がします。

どうでしょうか？

charon-r13b · 2025-02-12T09:02:41Z

src/main/java/nablarch/core/beans/BeanUtil.java

+            if (srcMap.isEmpty()) {
+                continue;
+            }
+            if (expression.isSimpleProperty() && isNode) {


この箇所ですが、呼び出される経路によってはisNodeとexpression.isNodeが異なる結果になることがありますか？

charon-r13b · 2025-02-12T10:04:04Z

src/main/java/nablarch/core/beans/BeanUtil.java

+                for (Map.Entry<String, ?> mapEntry : map.entrySet()) {
+                    PropertyExpression keyAbsoluteExpression = expression.sibling(mapEntry.getKey());
+                    PropertyExpression restExpression = keyAbsoluteExpression.rest();
+                    groupMap.computeIfAbsent(restExpression.getParentKey() + "." + restExpression.getRoot(),


restExpression.getParentKey() + "." + restExpression.getRoot()というパターンが頻出しますが、PropertyExpressionのメソッドにしてはどうでしょうか？

PropertyExpression#getAbsoluteRootなどでよい気がします。

別コメントで書いたように、集約されるのならこのままでもよいかもですが。

charon-r13b · 2025-02-12T11:03:09Z

src/main/java/nablarch/core/beans/PropertyExpression.java

+     *
+     * @return リーフ要素のPropertyExpression
+     */
+    PropertyExpression leaf() {


このメソッド、使用箇所がないように見えますが意図的に残されていますか？

charon-r13b · 2025-02-12T11:03:30Z

src/main/java/nablarch/core/beans/PropertyExpression.java

+     * @param propertyName　プロパティ名
+     * @return 同一親をもつPropertyExpression
+     */
+    PropertyExpression sibling(String propertyName) {


このメソッド、使用箇所がないように見えますが意図的に残されていますか？

charon-r13b · 2025-02-12T11:04:31Z

src/main/java/nablarch/core/beans/PropertyExpression.java

+        return parentKey;
+    }
+
+    String getAbsoluteRawKey() {


このメソッド、使用箇所がないように見えますが意図的に残されていますか？

charon-r13b · 2025-02-12T11:23:25Z

src/main/java/nablarch/core/beans/BeanUtil.java

+                    LOGGER.logDebug(
+                            "An error occurred while writing to the property :" + nestedExpression.getAbsoluteRawKey());


テストケースに、ここでの例外ハンドリング時にnestedExpression.getAbsoluteRawKey()の結果が期待通りか確認するテストが必要ではないでしょうか？

charon-r13b · 2025-02-12T11:26:45Z

いくつかのメソッドの引数に出てくるisNodeを、できればなくしたいです。

既存のメソッドで、単一の値を設定するものはsetPropertyValueになっているので同じように接尾語をValueにしてメソッドを呼び分けてはどうでしょうか。

charon-r13b · 2025-02-13T02:40:39Z

この修正の発端は、タイトルに書いてあるとおり「BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる」なので、その対応結果が必要です。

目安として10,000件程度での修正前後の性能比較が上がっていたと思いますが、その結果を概要に追記しておいてください。

morioka-homare added 6 commits February 6, 2025 10:18

fix: BeanUtilのcopyメソッドの修正でテストコード追加

94d38d4

fix: BeanUtilのcopyメソッドの修正でテストコード追加

fe240d5

fix: BeanUtilのcopyメソッド_大量オブジェクトで遅くなるを修正

696e867

fix: BeanUtilのcopyメソッド_大量オブジェクトで遅くなるを修正

bbe8f4c

fix: 単体テスト不具合の修正

be88122

fix: 微修正

8a10702

tisuchida reviewed Feb 7, 2025

View reviewed changes

morioka-homare added 4 commits February 7, 2025 18:18

fix: ログ出力用修正

4e57b5b

fix: 指摘対応など

cdcf1ae

fix: 多数オブジェクトの件、全体構成修正

9683abc

fix: テストコード追加、修正、Sonar対応

b22593a

charon-r13b reviewed Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる #40

BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる #40

morioka-homare commented Feb 6, 2025

tisuchida Feb 7, 2025

morioka-homare Feb 13, 2025

tisuchida Feb 7, 2025

tisuchida Feb 7, 2025

morioka-homare Feb 13, 2025

tisuchida Feb 7, 2025

morioka-homare Feb 13, 2025

tisuchida Feb 7, 2025

morioka-homare Feb 13, 2025

tisuchida Feb 7, 2025

charon-r13b Feb 12, 2025

morioka-homare Feb 13, 2025

tisuchida Feb 7, 2025

morioka-homare Feb 13, 2025

charon-r13b Feb 12, 2025

charon-r13b Feb 12, 2025

charon-r13b Feb 12, 2025

charon-r13b Feb 12, 2025 •

edited

Loading

charon-r13b Feb 12, 2025

charon-r13b Feb 12, 2025

charon-r13b Feb 12, 2025

charon-r13b Feb 12, 2025

charon-r13b commented Feb 12, 2025

charon-r13b commented Feb 13, 2025


		PropertyExpression expression = new PropertyExpression(entry.getKey());

		if (expression.isSimpleProperty() && expression.isNode()) {

		LOGGER.logDebug(
		"An error occurred while writing to the property :" + nestedExpression.getAbsoluteRawKey());

BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる #40

Are you sure you want to change the base?

BeanUtilでMapからBeanへ移送する際、ネストしたオブジェクト数が多い場合に処理が遅くなる #40

Conversation

morioka-homare commented Feb 6, 2025

不具合内容

修正内容

テスト

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charon-r13b Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charon-r13b commented Feb 12, 2025

charon-r13b commented Feb 13, 2025

charon-r13b Feb 12, 2025 •

edited

Loading