You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2021/06/24 17:43:44 UTC
[GitHub] [logging-log4j2] arturobernalg opened a new pull request #532: Replace loop with bulk method.
arturobernalg opened a new pull request #532:
URL: https://github.com/apache/logging-log4j2/pull/532
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] vy commented on a change in pull request #532: Replace loop with bulk method.
Posted by GitBox <gi...@apache.org>.
vy commented on a change in pull request #532:
URL: https://github.com/apache/logging-log4j2/pull/532#discussion_r658172667
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/util/TypeUtil.java
##########
@@ -53,9 +54,7 @@ private TypeUtil() {
public static List<Field> getAllDeclaredFields(Class<?> cls) {
final List<Field> fields = new ArrayList<>();
while (cls != null) {
- for (final Field field : cls.getDeclaredFields()) {
- fields.add(field);
- }
+ fields.addAll(Arrays.asList(cls.getDeclaredFields()));
Review comment:
I have the impression that we are adding an extra GC load to a commonly used part of the code base. I'd prefer the earlier approach. Would you mind reverting this, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #532: Replace loop with bulk method.
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #532:
URL: https://github.com/apache/logging-log4j2/pull/532#discussion_r658181865
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/util/TypeUtil.java
##########
@@ -53,9 +54,7 @@ private TypeUtil() {
public static List<Field> getAllDeclaredFields(Class<?> cls) {
final List<Field> fields = new ArrayList<>();
while (cls != null) {
- for (final Field field : cls.getDeclaredFields()) {
- fields.add(field);
- }
+ fields.addAll(Arrays.asList(cls.getDeclaredFields()));
Review comment:
I don't think it's entirely clear which one will perform better, but I agree with @vy that the original is likely to outperform addAll.
The benefit of addAll is that the receiving collection doesn't need to resize the underlying array multiple times, however ArrayList.addAll(Collection) calls Collection.toArray(), and Arrays.asList(arr) uses an implementation which produces a copy of the underlying array on each call. That's expensive, but we're deep enough into hot methods that C2 may be able to help us out by avoiding the duplicate array allocation (potentially including the getDeclaredFields() array).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] arturobernalg commented on a change in pull request #532: Replace loop with bulk method.
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #532:
URL: https://github.com/apache/logging-log4j2/pull/532#discussion_r658178940
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/util/TypeUtil.java
##########
@@ -53,9 +54,7 @@ private TypeUtil() {
public static List<Field> getAllDeclaredFields(Class<?> cls) {
final List<Field> fields = new ArrayList<>();
while (cls != null) {
- for (final Field field : cls.getDeclaredFields()) {
- fields.add(field);
- }
+ fields.addAll(Arrays.asList(cls.getDeclaredFields()));
Review comment:
Done.
TY
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org