Skip to content

introduce a new Input/Output: FileInput/FileOutput #5515

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 25, 2020
Merged

introduce a new Input/Output: FileInput/FileOutput #5515

merged 1 commit into from
Nov 25, 2020

Conversation

jtjeferreira
Copy link
Contributor

@jtjeferreira jtjeferreira commented Apr 26, 2020 •
edited
Loading

  • FileInput avoids the intermediate String parsing of PlainInput
  • FileOuput also avoid the intermediate String representation of PlainOutput

jtjeferreira
jtjeferreira commented Apr 26, 2020
View reviewed changes
util-cache/src/main/scala/sbt/util/Input.scala
@@ -44,3 +45,13 @@ class PlainInput[J: IsoString](input: InputStream, converter: SupportConverter[J

def close() = input.close()
}

class FileInput(file: File) extends Input {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this in the new Input that uses Parser.parseFromFile directly , instead of IsoString to convert to a interdemiary String first. This is where this whole PR started...

util-cache/src/main/scala/sbt/util/CacheStore.scala
@@ -23,14 +24,15 @@ abstract class CacheStore extends Input with Output {
}

object CacheStore {
@deprecated("Create your own IsoString[JValue]", "1.4")
implicit lazy val jvalueIsoString: IsoString[JValue] =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this val is kept just for bincompt...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to deprecate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it can be removed in the future

util-cache/src/main/scala/sbt/util/CacheStore.scala
@@ -47,38 +49,44 @@ abstract class CacheStoreFactory {
}

object CacheStoreFactory {
@deprecated("Create your own IsoString[JValue]", "1.4")
implicit lazy val jvalueIsoString: IsoString[JValue] =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this val is kept just for bincompt...

util-cache/src/main/scala/sbt/util/CacheStore.scala Outdated
IO.createDirectory(base)

def make(identifier: String): CacheStore = new FileBasedStore(base / identifier, converter)
@deprecated("Use constructor without converter", "1.4")
def this(base: File, converter: sjsonnew.SupportConverter[J], isoString: sjsonnew.IsoString[J]) =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constructor was added, just for bincompt, but some parameters will just be ignored...

util-cache/src/main/scala/sbt/util/CacheStore.scala
IO.touch(file, setModified = false)

@deprecated("Use constructor without converter", "1.4")
def this(file: File, converter: sjsonnew.SupportConverter[J])(implicit e: sjsonnew.IsoString[J]) =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this constructor was added, just for bincompt, but some parameters will just be ignored...

@jtjeferreira
Copy link
Contributor Author

tests are failing on windows... any help with those?

@eed3si9n
Copy link
Member

https://github.com/sbt/sbt/blob/develop/sbt/src/sbt-test/actions/previous/build.sbt

[info] Running actions/previous
[info] [error] java.lang.AssertionError: assertion failed: Expected 'a' to be 1, got 8
[info] [error] 	at scala.Predef$.assert(Predef.scala:223)
[info] [error] 	at $5e6b47047a4c8d8f03cb$.$anonfun$$sbtdef$3(C:\Users\appveyor\AppData\Local\Temp\1\sbt_19336570\build.sbt:30)
[info] [error] 	at $5e6b47047a4c8d8f03cb$.$anonfun$$sbtdef$3$adapted(C:\Users\appveyor\AppData\Local\Temp\1\sbt_19336570\build.sbt:26)
[info] [error] 	at scala.Function1.$anonfun$compose$1(Function1.scala:49)
[info] [error] 	at sbt.internal.util.$tilde$greater.$anonfun$$u2219$1(TypeFunctions.scala:62)
[info] [error] 	at sbt.std.Transform$$anon$4.work(Transform.scala:67)
[info] [error] 	at sbt.Execute.$anonfun$submit$2(Execute.scala:282)
[info] [error] 	at sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:23)
[info] [error] 	at sbt.Execute.work(Execute.scala:291)
[info] [error] 	at sbt.Execute.$anonfun$submit$1(Execute.scala:282)
[info] [error] 	at sbt.ConcurrentRestrictions$$anon$4.$anonfun$submitValid$1(ConcurrentRestrictions.scala:184)
[info] [error] 	at sbt.CompletionService$$anon$2.call(CompletionService.scala:41)
[info] [error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] [error] 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[info] [error] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info] [error] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[info] [error] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[info] [error] 	at java.lang.Thread.run(Thread.java:748)
[info] [error] (checkNext) java.lang.AssertionError: assertion failed: Expected 'a' to be 1, got 8
[error] x actions/previous 
[error]  Cause of test exception: {line 6}  Command failed: checkNext failed

so for some reason clean didn't have the effect of clearing out the previous value?

@jtjeferreira
Copy link
Contributor Author

fixed the windows error which was caused by typelevel/jawn#109, however now the build fails in 2.13 because in 2.13 we use jawn 1.0...

@jtjeferreira
Copy link
Contributor Author

finally tests are green

eed3si9n
eed3si9n reviewed Apr 28, 2020
View reviewed changes
eed3si9n
eed3si9n reviewed Apr 28, 2020
View reviewed changes
main/src/main/scala/sbt/internal/LibraryManagement.scala
Comment on lines -320 to +314
val store = new FileBasedStore(file, sjsonnew.support.scalajson.unsafe.Converter)
val store = new FileBasedStore(file)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see difference in load time from this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I havent benchmarked this change against my sample project in #5508...

Do you think I should add that sample project to a scripted test so is easier to check? or is it too much for a unit test?

Copy link
Member

@eed3si9n eed3si9n Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok for the tests to prove just the correctness, but if we're making some optimization or changes aimed at performance improvement, I would like to see some data even if it's casually done local testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eed3si9n! I rebased this PR and compared the performance of this PR versus 1.4.0-M1 and did not see any significant improvements. I can try to write some jmh benchmarks, but I see there aren't benchmarks in the project...

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a mixed feeling about jmh. In general they are useful. But since the Scala compiler is complicated, even with 10 warmup or whatever C2 would keep optimizing for like 10 minutes, even longer.. so that means that more iteration you run, the performance gets better at least for compilation. See sbt/zinc#767 (comment). This might affect sbt indirectly since there are some aspect of compilation. So to minimize the fluctuation, we should reuse *.class files between the iteration?

Existing code that might be relevant for benchmarking is scripted test and server test.

See also https://github.com/eatkins/scala-build-watch-performance

@jtjeferreira
Copy link
Contributor Author

Hi @eed3si9n

I rebased this PR against 1.4.4 and compare these changes against it with https://github.com/jtjeferreira/sbt-multi-module-sample

I see some improvements in FileInput vs PlainInput (872 vs 1052 samples in the flamegraph), but no improvements in FileOutput vs PlainOutput (both around 240 samples in the flamegraph).

WDYT? should we merge? should we close? should we benchmark differently?

jtjeferreira added a commit to jtjeferreira/sbt-multi-module-sample that referenced this pull request Nov 23, 2020
@jtjeferreira
add a few flamegraphs for sbt/sbt#5515
c6a288f
@jtjeferreira
Copy link
Contributor Author

the flamegraphs can be found in this commit jtjeferreira/sbt-multi-module-sample@c6a288f

@eed3si9n
Copy link
Member

eed3si9n commented Nov 23, 2020 •
edited
Loading

I see some improvements in FileInput vs PlainInput (872 vs 1052 samples in the flamegraph), but no improvements in FileOutput vs PlainOutput (both around 240 samples in the flamegraph).

Thanks for the candid assessment. As a general idea streaming bytes directly into files sounds like a good idea. What do you think about merging this to develop, targeting for sbt 1.5.0? (Planned to be out early 2021, as opposed to the normal year-long wait)

If that's ok, could you squash the commits plz?

@jtjeferreira jtjeferreira force-pushed the better_file_input branch 2 times, most recently from 5dd22d3 to dd411ca Compare November 25, 2020 14:04
@jtjeferreira
introduce a new new Input/Output: FileInput/FileOutput
dd411ca 
* FileInput avoids the intermediate String parsing of PlainInput
* FileOuput also avoid the intermediate String representation of PlainOutput
@jtjeferreira jtjeferreira changed the title introduce a new new Input: FileInput introduce a new Input/Output: FileInput/FileOutput Nov 25, 2020
@jtjeferreira
Copy link
Contributor Author

I see some improvements in FileInput vs PlainInput (872 vs 1052 samples in the flamegraph), but no improvements in FileOutput vs PlainOutput (both around 240 samples in the flamegraph).

Thanks for the candid assessment. As a general idea streaming bytes directly into files sounds like a good idea. What do you think about merging this to develop, targeting for sbt 1.5.0? (Planned to be out early 2021, as opposed to the normal year-long wait)

If that's ok, could you squash the commits plz?

looks good to me. the commits were squashed.

@eed3si9n eed3si9n merged commit 1051690 into sbt:develop Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eed3si9n eed3si9n eed3si9n approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jtjeferreira @eed3si9n