You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pdfbox.apache.org by "cinsttool (via GitHub)" <gi...@apache.org> on 2024/04/01 08:58:56 UTC
[PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
cinsttool opened a new pull request, #188:
URL: https://github.com/apache/pdfbox/pull/188
Hi,
We find that there exist container inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java.
In particular, at line 301 of Type1CharString.java, the object `list` copies the `numbers` and removes the last two elements for calculation purposes before adding the result, which is then returned by the method `handleType1Command`. Apart from this scenario, method `handleType1Command` returns `Collections.emptyList()`. However, at lines 154 and 155, after the the invocation of `handleType1Command`, the `numbers` object is cleared, and the `results` object are copied to the `numbers` object using the `addAll` method. This process involves redundant copying and clearing operations.
For COSWriterCompressionPool.java, at the line 56, the created `List` object `allDirectObjects` is only used for `contains` and `add` operations within the `addElements` method. Such usage scenarios are more suitable for a `Set` container with lower complexity.
For ShadedTriangle.java, according to the prurpose of method calcDeg, there is too expensive to create objects and pass them to a `Set` to obtain the count of unique elements and then returning the count.
We discovered the above containers inefficiencies by our tool cinst. The patch is submitted. Could you please check and accept it? We have tested the patch on our PC. The patched program works well.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "lehmi (via GitHub)" <gi...@apache.org>.
lehmi commented on PR #188:
URL: https://github.com/apache/pdfbox/pull/188#issuecomment-2054005153
My bad, I missed one line. Finally everything is fine now
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "cinsttool (via GitHub)" <gi...@apache.org>.
cinsttool commented on PR #188:
URL: https://github.com/apache/pdfbox/pull/188#issuecomment-2029482338
> I'm undecided about the first one, I like the second, I don't like the third one because the code looks much more complex. I assume you're hard-coding the usage of the 3-set.
Yes, we only evaluate container usage patterns from a performance perspective. For example, regarding the third one, we consider that using a container solely to obtain a size instead of persistently storing and managing objects may be too costly. However, the modification we provided may indeed be less concise and clear compared to the original implementation.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #188: Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java
URL: https://github.com/apache/pdfbox/pull/188
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "lehmi (via GitHub)" <gi...@apache.org>.
lehmi commented on PR #188:
URL: https://github.com/apache/pdfbox/pull/188#issuecomment-2054003440
All tests passed withount any error, but the rendering is broken. I'm going to have a look
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "THausherr (via GitHub)" <gi...@apache.org>.
THausherr commented on PR #188:
URL: https://github.com/apache/pdfbox/pull/188#issuecomment-2029457536
I'm undecided about the first one, I like the second, I don't like the third one because the code looks much more complex. I assume you're hard-coding the usage of the 3-set.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "cinsttool (via GitHub)" <gi...@apache.org>.
cinsttool commented on code in PR #188:
URL: https://github.com/apache/pdfbox/pull/188#discussion_r1546241225
##########
fontbox/src/main/java/org/apache/fontbox/cff/Type1CharString.java:
##########
@@ -161,15 +159,15 @@ private void render()
});
}
- private List<Number> handleType1Command(List<Number> numbers, CharStringCommand command)
+ private void handleType1Command(List<Number> numbers, CharStringCommand command)
{
commandCount++;
Type1KeyWord type1KeyWord = command.getType1KeyWord();
if (type1KeyWord == null)
{
// indicates an invalid charstring
LOG.warn("Unknown charstring command in glyph {} of font {}", glyphName, fontName);
- return Collections.emptyList();
+ numbers.clear();
Review Comment:
Sorry for the mistake! We push this patch.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "cinsttool (via GitHub)" <gi...@apache.org>.
cinsttool commented on PR #188:
URL: https://github.com/apache/pdfbox/pull/188#issuecomment-2029547059
> > I'm undecided about the first one, I like the second, I don't like the third one because the code looks much more complex. I assume you're hard-coding the usage of the 3-set.
>
> Yes, we only evaluate container usage patterns from a performance perspective. For example, regarding the third one, we consider that using a container solely to obtain a size instead of persistently storing and managing objects may be too costly. However, the modification we provided may indeed be less concise and clear compared to the original implementation.
Maybe we can set initial capacity of the set to 3 that can save memory consumption while maintain the usage of the 3-set as the original implementation.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "lehmi (via GitHub)" <gi...@apache.org>.
lehmi commented on code in PR #188:
URL: https://github.com/apache/pdfbox/pull/188#discussion_r1546231761
##########
fontbox/src/main/java/org/apache/fontbox/cff/Type1CharString.java:
##########
@@ -161,15 +159,15 @@ private void render()
});
}
- private List<Number> handleType1Command(List<Number> numbers, CharStringCommand command)
+ private void handleType1Command(List<Number> numbers, CharStringCommand command)
{
commandCount++;
Type1KeyWord type1KeyWord = command.getType1KeyWord();
if (type1KeyWord == null)
{
// indicates an invalid charstring
LOG.warn("Unknown charstring command in glyph {} of font {}", glyphName, fontName);
- return Collections.emptyList();
+ numbers.clear();
Review Comment:
The immediate return is missing, so that the switch runs into the default case and throws an exception
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org
Re: [PR] Fix containers inefficiencies in Type1CharString.java, COSWriterCompressionPool.java, and ShadedTriangle.java [pdfbox]
Posted by "lehmi (via GitHub)" <gi...@apache.org>.
lehmi commented on PR #188:
URL: https://github.com/apache/pdfbox/pull/188#issuecomment-2053980187
@cinsttool thanks for the contribution. I've applied the remaining changes for Type1CharString
--
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.
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pdfbox.apache.org
For additional commands, e-mail: dev-help@pdfbox.apache.org