You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by xnslong <gi...@git.apache.org> on 2017/07/11 10:01:01 UTC
[GitHub] logging-log4j2 pull request #92: consider the StringBuilder's capacity inste...
GitHub user xnslong opened a pull request:
https://github.com/apache/logging-log4j2/pull/92
consider the StringBuilder's capacity instead of content length when do the trim
The trim operation aims at releasing the too much memory occupied by the `StringBuilder`. So when the StringBuilder really contains few effective characters, but with large capacity, should also be trimmed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/xnslong/logging-log4j2 master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/logging-log4j2/pull/92.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #92
----
commit d61379bc66b977f53f51e0c5caeee7193aca1f32
Author: Jerry <sz...@qq.com>
Date: 2017-07-11T09:15:36Z
Merge pull request #1 from apache/master
pull latest changes from base
commit ab07e40e178e62e07234d45a056770370be7e1cc
Author: xnslong <xn...@outlook.com>
Date: 2017-07-11T09:40:21Z
StringBuilders determine if trim is necessary on StringBuilders capacity instead of content length
commit 9db3f5115e2eb047562fcf4c1e34fa1d7c8e4e86
Author: xnslong <xn...@outlook.com>
Date: 2017-07-11T09:56:38Z
update test case
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
I runned this test, but it didn't fail on my machine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
I reappeared the failure! when I run the tests with `mvn clean test -pl log4j2-core`, I got the following failure (it is just the failure you provided)
```
[ERROR] Failures:
[ERROR] AbstractStringLayoutTest.testGetStringBuilderCapacityRestrictedToMax:74 capacity, trimmed to MAX_STRING_BUILDER_SIZE expected:<2048> but was:<4096>
```
Then I run mvn in debug mode, and find out the `log4j2-api` is not my local code, but some code downloaded from the maven snapshot repository.
> command for running test in debug mode:
>
> ```
> mvn test -pl log4j-core -Dmaven.surefire.debug="-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=8000 -Xnoagent -Djava.compiler=NONE" -Dtest=AbstractStringLayoutTest
> ```
Well, the correct command for running the test is `mvn clean test -am -pl log4j2-core`. It will compile `log4j2-core` and all its dependent modules, but I didn't have java9 installed, so it will always fail for the `log4j-api-java9`. If you have java9 installed, please check it to see whether it fails.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
Please accept my apologies, I made a mistake saying the patch was partly applied to log4j-api. It was not. The new test passes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
With this patch, I now get:
[ERROR] Failures:
[ERROR] AsyncLoggerThreadContextGarbageFreeTest>AbstractAsyncThreadContextTestBase.testAsyncLogWritesToLog:159->AbstractAsyncThreadContextTestBase.checkResult:185 AsyncLoggerTest.log: line 18 expected:<...yncLoggerContext i=1[]8> but was:<...yncLoggerContext i=1[2]8>
[INFO]
[ERROR] Tests run: 1889, Failures: 1, Errors: 0, Skipped: 21
[INFO]
[INFO] ------------------------------------------------------------------------
Gary
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 pull request #92: Consider the StringBuilder's capacity inste...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/logging-log4j2/pull/92
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 pull request #92: Consider the StringBuilder's capacity inste...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on a diff in the pull request:
https://github.com/apache/logging-log4j2/pull/92#discussion_r127092555
--- Diff: log4j-core/src/test/java/org/apache/logging/log4j/core/layout/AbstractStringLayoutTest.java ---
@@ -59,12 +59,14 @@ public void testGetStringBuilderCapacityRestrictedToMax() throws Exception {
final int LARGE = 4096;
final String largeMessage = new String(new char[LARGE]);
final StringBuilder sb2 = ConcreteStringLayout.getStringBuilder();
- assertEquals("resized capacity", GROWN, sb2.capacity());
+ assertTrue("reset capacity", sb2.capacity() <= AbstractStringLayout.MAX_STRING_BUILDER_SIZE);
--- End diff --
Can't we have an exact comparison here? This feels hacky to me :-(
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
@garydgregory Yes, I noticed that and fixed it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
I followed the tips to run command `mvn -U clean test -pl log4j-core -Dtest=AbstractStringLayoutTest`, and then I checked my local maven repository, but I'm afraid it didn't work. The following picture is a screen shot when I decompile the `log4j-api-2.9-SNAPSHOT.jar` with `Java Decompiler`. Please have a look on the red rectangle I draw here, it is where different from my code, and also the reason cause the failures above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
I merged the latest master to my branch just now, Would you please run the case again, and show me the result? @garydgregory
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
Still reports this problem? Hmm... interesting. I also have ran this case in my IDE, nothing went wrong. Would you please show me the git repository version, maybe we have to check if we are running the same version of code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
I have the whole project sources and all modules (sources) loaded in
Eclipse so I run the one test directly from Eclipse.
The commands you show will never work (did you try them?) since there is no
log4j2-core module, it is log4j-core.
So if I run:
mvn clean install -pl log4j-api
and:
mvn clean test -pl log4j-core -Dtest=AbstractStringLayoutTest
I get:
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.logging.log4j.core.layout.AbstractStringLayoutTest
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed:
0.032 s <<< FAILURE! - in
org.apache.logging.log4j.core.layout.AbstractStringLayoutTest
[ERROR]
testGetStringBuilderCapacityRestrictedToMax(org.apache.logging.log4j.core.layout.AbstractStringLayoutTest)
Time elapsed: 0.032 s <<< FAILURE!
java.lang.AssertionError: capacity, trimmed to MAX_STRING_BUILDER_SIZE
expected:<2048> but was:<4096>
at
org.apache.logging.log4j.core.layout.AbstractStringLayoutTest.testGetStringBuilderCapacityRestrictedToMax(AbstractStringLayoutTest.java:74)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR]
AbstractStringLayoutTest.testGetStringBuilderCapacityRestrictedToMax:74
capacity, trimmed to MAX_STRING_BUILDER_SIZE expected:<2048> but was:<4096>
[INFO]
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0
[INFO]
[INFO]
------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO]
------------------------------------------------------------------------
[INFO] Total time: 53.099 s
[INFO] Finished at: 2017-07-17T08:53:05-07:00
[INFO] Final Memory: 43M/686M
Make sure you have the latest from git master.
Thank you!
Gary
On Mon, Jul 17, 2017 at 6:09 AM, Jerry <no...@github.com> wrote:
> I reappeared the failure! when I run the tests with mvn clean test -pl
> log4j2-core, I got the following failure (it is just the failure you
> provided)
>
> [ERROR] Failures:
> [ERROR] AbstractStringLayoutTest.testGetStringBuilderCapacityRestrictedToMax:74 capacity, trimmed to MAX_STRING_BUILDER_SIZE expected:<2048> but was:<4096>
>
> Then I run mvn in debug mode, and find out the log4j2-api is not my local
> code, but some code downloaded from the maven snapshot repository.
>
> command for running test in debug mode:
>
> mvn test -pl log4j-core -Dmaven.surefire.debug="-Xdebug -Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=8000 -Xnoagent -Djava.compiler=NONE" -Dtest=AbstractStringLayoutTest
>
> Well, the correct command for running the test is mvn clean test -am -pl
> log4j2-core. It will compile log4j2-core and all its dependent modules,
> but I didn't have java9 installed, so it will always fail for the
> log4j-api-java9. If you have java9 installed, please check it to see
> whether it fails.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/logging-log4j2/pull/92#issuecomment-315750578>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABIfN_X6J_VNqM93qO3459iVX2_RUJHLks5sO1z0gaJpZM4OUANs>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: [GitHub] logging-log4j2 issue #92: Consider the StringBuilder's
capacity instead of c...
Posted by Gary Gregory <ga...@gmail.com>.
Hi,
If you want your log4j-core checkout to use changes you make in log4j-api
locally, then you MUST install log4j-api in your local repository (see the
mvn commands I provided). BUT...
In this case, the log4j-api changes are already in git master. If you want
to make sure you have the latest SNAPSHOT version of jars, you can force
Maven to update with the -U option:
mvn -U clean test -pl log4j-core -Dtest=AbstractStringLayoutTest
You should _not_ need to build log4j-api since the length() change is
already in git master. I forgot that we already had the log4j-api change in
git master, so I am sorry for that mixup.
In my case, I have everything checked out and built locally.
I build and install log4j-api, then test log4j-core (all on the command
line), then there is no downloading of log4j-api because what I have
locally is the newer than anything and Maven is not checking remotely for a
newer version. If I waited longer, it is possible that Maven might pick a
version from a remote repo, I am not sure if Maven works that way.
This should not matter in this case since the log4j-api changes are already
in git master and therefore should be reflected in any modules downloaded
remotely.
Now here is another issue: Over the weekend, our CI system went through a
major upgrade and we've had build failures. Maybe that causes old versions
to still be served. To avoid any potential issues, I just deployed a fresh
log4j-api to the SNAPSHOT repo (
https://repository.apache.org/content/repositories/snapshots/)
Gary
On Mon, Jul 17, 2017 at 9:05 AM, xnslong <gi...@git.apache.org> wrote:
> Github user xnslong commented on the issue:
>
> https://github.com/apache/logging-log4j2/pull/92
>
> After this failure, could you please check your local maven repository
> for the log4j-api-2.9-SNAPSHOT version, decompile the jar and check the
> StringBuilders class. After I got the failure, I found it is not compiled
> from my code.
>
> I didn't check the command I provided above, I just wrote it on my
> experience. My point is that the two modules may have to be compiled in
> only 1 command. For me, my settings for maven will download the dependent
> SNAPSHOT artifacts every time I build a module, so even when I installed
> the SNAPSHOT version, it will be rewritten when I try to build the other
> module. I'm not sure what's your settings, so I suggest to check the jar
> file in the repository.
>
> ```java
> public static void trimToMaxSize(final StringBuilder stringBuilder,
> final int maxSize) {
> if (stringBuilder != null && stringBuilder.capacity() > maxSize) {
> // I modified this line, but I found it remains unchanged and still check
> the stringBuilder.length() in the decompiled code.
> stringBuilder.setLength(maxSize);
> stringBuilder.trimToSize();
> }
> }
> ```
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
After this failure, could you please check your local maven repository for the log4j-api-2.9-SNAPSHOT version, decompile the jar and check the StringBuilders class. After I got the failure, I found it is not compiled from my code.
I didn't check the command I provided above, I just wrote it on my experience. My point is that the two modules may have to be compiled in only 1 command. For me, my settings for maven will download the dependent SNAPSHOT artifacts every time I build a module, so even when I installed the SNAPSHOT version, it will be rewritten when I try to build the other module. I'm not sure what's your settings, so I suggest to check the jar file in the repository.
```java
public static void trimToMaxSize(final StringBuilder stringBuilder, final int maxSize) {
if (stringBuilder != null && stringBuilder.capacity() > maxSize) { // I modified this line, but I found it remains unchanged and still check the stringBuilder.length() in the decompiled code.
stringBuilder.setLength(maxSize);
stringBuilder.trimToSize();
}
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
Hi,
When I build with 'mvn clean install' from a clean git master, all the tests pass. Note that I am on Windows 10 and that the test HttpAppenderTest fails randomly for me, so I usually annotate it with @Ignored in my local workspace. It looks like you have a debugging session ahead of you in your favorite IDE ;-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #92: Consider the StringBuilder's capacity instead of c...
Posted by xnslong <gi...@git.apache.org>.
Github user xnslong commented on the issue:
https://github.com/apache/logging-log4j2/pull/92
Well, the case runs well on my computer. But I notice that the `MAX_STRING_BUILDER_SIZE` is not a constant, instead it may be loaded from the sysstem properties. So I changed my test case, to define the size upon the value of `MAX_STRING_BUILDER_SIZE`, instead of a hard coded number. Let's see, whether it will work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---