Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

UseBufferedInputStream withFileInputStream#3750

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vlsi merged 3 commits intopgjdbc:masterfromjgardn3r:buffered-readers
Aug 12, 2025

Conversation

jgardn3r
Copy link
Contributor

Some places whereFileInputStream was being used didn't haveBufferedInputStream. In many of the use cases, these files were being read 1 byte at a time. This causes excessive IO calls to read the file. By ensuring theInputStream is buffered, the IO calls are minimised, improving the performance of the driver.

This performance issue was noticed when a certificate file was on a network drive. It resulted in considerable network traffic.

All Submissions:

  • Have you followed the guidelines in ourContributing document?
  • Have you checked to ensure there aren't other openPull Requests for the same update/change?

Some places where `FileInputStream` was being used didn't have`BufferedInputStream`. In many of the use cases, these files were beingread 1 byte at a time. This causes excessive IO calls to read the file.By ensuring the `InputStream` is buffered, the IO calls are minimised,improving the performance of the driver.
@vlsi
Copy link
Member

vlsi commentedAug 5, 2025
edited
Loading

For reference, it looks likeFiles.newInputStream could be the way to go (it does not seem to requireBufferedInputStream), however, it does not seem to be good enough for Java 8.

See:

At the same time,Files.newInputStream says "the stream will not be buffered", so it would require buffering anyways.

@davecramer
Copy link
Member

Interesting. Thanks for the PR

jgardn3r reacted with heart emoji

@vlsi
Copy link
Member

vlsi commentedAug 6, 2025

I've an idea how we could prevent similar issues in the future:

  1. Createorg.postgresql.util.internal.IoUtil class withBufferedInputStream newBufferedInputStream(File) method
  2. Make sure we use the method whenever we neednew FileInputStream(...)
  3. Addjava.io.FileInputStream#FileInputStream(java.io.File) andFileInputStream(String name) constructors toforbidden-api.txt:
    @defaultMessage Use toLowerCase(Locale.ROOT) and toUpperCase(Locale.ROOT)
  4. Excludeorg.postgresql.util.internal.IoUtil from forbidden-apis check:
    exclude("**/org/postgresql/util/internal/Unsafe.class")

Then the build would preventnew FileInputStream entering to the source code.

WDYT?

davecramer reacted with thumbs up emoji

@vlsi
Copy link
Member

vlsi commentedAug 6, 2025

@jgardn3r , would you be open to update the PR as per#3750 (comment) ?

jgardn3r reacted with thumbs up emoji

@jgardn3r
Copy link
ContributorAuthor

@jgardn3r , would you be open to update the PR as per#3750 (comment) ?

Happy to make the changes.

A few things that I have thought while implementing these changes:

  • I'm not a huge fan of the signatureIoUtils.newBufferedInputStream(File) (because, of course, naming is hard)
    • I think if I were naming it, I would call itFileUtils.newInputStream(File) similar to that of theFiles.newInputStream()
      The reason being: if you are always going to expect file streams to be buffered, then there is no real point to put it in the name. If you want a file stream, it will be buffered, whether you like it or not.
    • There are many uses where the argument is aString, so I would add an overload for that
  • The exclusions list references an imaginaryUnsafe.class file. Should I replace that with the new file?
  • Should test code be excluded or updated to use the new util?

What do you guys think?

@vlsi
Copy link
Member

vlsi commentedAug 7, 2025

FileUtils is indeed a better class name.

The reason being: if you are always going to expect file streams to be buffered, then there is no real point to put it in the name. If you want a file stream, it will be buffered, whether you like it or not

Well,Files.newBufferedReader does haveBuffered in the name. I just thought having.newBufferedInputStream would would convey the intent.

The exclusions list references an imaginaryUnsafe.class file. Should I replace that with the new file?

Let's replace it indeed.

Should test code be excluded or updated to use the new util?

AFAIK there are two instances, and they are worth updating:

new StrangeInputStream(seed, new FileInputStream("target/buffer.txt"));

It is fine to convert to a new util method. It looks like a true case when the necessary buffering was missing.

new BufferedReader(new InputStreamReader(new FileInputStream(path)));

It could rather beFiles.newBufferedReader(...) instead

jgardn3r reacted with thumbs up emoji

@vlsi
Copy link
Member

vlsi commentedAug 7, 2025

For reference, I've filedopenrewrite/rewrite-static-analysis#694 so openrewrite could automatically replacenew BufferedReader(new InputStreamReader(new FileInputStream(path))) withFiles.newBufferedReader in the future.

jgardn3r reacted with thumbs up emoji

@vlsivlsi merged commit34a3416 intopgjdbc:masterAug 12, 2025
18 checks passed
@jgardn3rjgardn3r deleted the buffered-readers branchAugust 13, 2025 02:05
svc-squareup-copybara pushed a commit to cashapp/misk that referenced this pull requestSep 19, 2025
| Package | Type | Package file | Manager | Update | Change ||---|---|---|---|---|---|| [org.postgresql:postgresql](https://jdbc.postgresql.org)([source](https://github.com/pgjdbc/pgjdbc)) | dependencies |misk/gradle/libs.versions.toml | gradle | patch | `42.7.7` -> `42.7.8` ||[com.google.apis:google-api-services-cloudkms](http://nexus.sonatype.org/oss-repository-hosting.html)([source](http://svn.sonatype.org/spice/tags/oss-parent-7)) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`v1-rev20250818-2.0.0` -> `v1-rev20250911-2.0.0` ||[org.assertj:assertj-core](https://assertj.github.io/doc/#assertj-core)([source](https://github.com/assertj/assertj)) | dependencies |misk/gradle/libs.versions.toml | gradle | patch | `3.27.4` -> `3.27.5` || [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:s3](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:regions](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` ||[software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava)| dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` || [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) |dependencies | misk/gradle/libs.versions.toml | gradle | patch |`2.33.12` -> `2.33.13` |---### Release Notes<details><summary>pgjdbc/pgjdbc (org.postgresql:postgresql)</summary>###[`v42.7.8`](https://github.com/pgjdbc/pgjdbc/blob/HEAD/CHANGELOG.md#4278-2025-09-18)##### Added- feat: Add configurable boolean-to-numeric conversion for ResultSetgetters [PR #&#8203;3796](pgjdbc/pgjdbc#3796)##### Changed- perf: remove QUERY\_ONESHOT flag when calling getMetaData [PR#&#8203;3783](pgjdbc/pgjdbc#3783)- perf: use `BufferedInputStream` with `FileInputStream` [PR#&#8203;3750](pgjdbc/pgjdbc#3750)- perf: enable server-prepared statements for DatabaseMetaData##### Fixed- fix: avoid NullPointerException when cancelling a query if cancel keyis not known yet- fix: Change "PST" timezone in TimestampTest to "Pacific Standard Time"[PR #&#8203;3774](pgjdbc/pgjdbc#3774)- fix: traverse the current dimension to get the correct pos inPgArray#calcRemainingDataLength [PR#&#8203;3746](pgjdbc/pgjdbc#3746)- fix: make sure getImportedExportedKeys returns columns in consistentorder- fix: Add "SELF\_REFERENCING\_COL\_NAME" field to getTables'ResultSetMetaData to fix NullPointerException [PR#&#8203;3660](pgjdbc/pgjdbc#3660)- fix: unable to open replication connection to servers < 12- fix: avoid closing statement caused by driver's internalResultSet#close()- fix: return empty metadata for empty catalog names as it was before- fix: Incorrect class comparison in PGXmlFactoryFactory validation</details>---### Configuration📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2amevery weekday" in timezone Australia/Melbourne, Automerge - At any time(no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Never, or you tick the rebase/retry checkbox.👻 **Immortal**: This PR will be recreated if closed unmerged. Get[config help](https://github.com/renovatebot/renovate/discussions) ifthat's undesired.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [RenovateBot](https://github.com/renovatebot/renovate).GitOrigin-RevId: 60823b502aee625ad34c64abe2d25a884fd81fee
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@jgardn3r@vlsi@davecramer

[8]ページ先頭

©2009-2025 Movatter.jp