Use String as the SourceFile content type - #26846
Conversation
| * the source is read from Tasty. */ | ||
| def content(): Array[Char] = | ||
| if file == null then Array.emptyCharArray | ||
| override def textContent(): String = |
There was a problem hiding this comment.
text is a shorter name, in case you are not content with this longer name.
There was a problem hiding this comment.
I have a grand unified String theory that explains why this is good. It's a little complex though.
|
Should we be bolder and use an The string compaction works if the entire string is latin1. A single non-latin1 character in a source file (perhaps in a license header that's repeated in all files) destroys it. |
|
We could, perhaps with some AnyVal derivative that hides the exact algorithms, but wouldn't that need a lot of changes in the compiler so anything that is used alongside file contents is also an |
|
Yeah, probably. It might be worth thinking about a larger changeset for the future. |
This comment was marked as outdated.
This comment was marked as outdated.
56d97b6 to
243d057
Compare
This comment was marked as outdated.
This comment was marked as outdated.
| for vars <- ctx.property(ShownVars) do vars += this | ||
| if !ctx.settings.YccDebug.value then "" | ||
| else if isConst then ids ++ "(solved)" | ||
| else if isConst then ids + "(solved)" |
There was a problem hiding this comment.
Benchmarking and profiling this PR turned into a general "let me look at what String-related stuff pops up" exercise.
We can't inline StringOps because the compiler might run with a different standard library.
Also, IMHO, it's weird to use ++ on strings when we're not explicitly looking at them as sequences of characters.
| idx != -1 | ||
| do | ||
| lineCount += 1 | ||
| buf.writeNat(lineCount) |
There was a problem hiding this comment.
this one comes from Haoyi's pickling-related perf PR. (I did not make the other changes to this function from that PR, namely inlining indexOf, because now that the content is a string, indexOf is a Java function, not a Scala stdlib extension like it is on arrays.)
| def sourceChangeContext(addr: Addr = currentAddr)(using Context): Context = { | ||
| val path = sourcePathAt(addr) | ||
| if (path.nonEmpty) { | ||
| if (!path.isEmpty) { |
There was a problem hiding this comment.
nonEmpty is a Scala extension
| /** Do two target names match? An empty target name matches any other name. */ | ||
| def matchesTargetName(other: Name) = | ||
| name == other || name.isEmpty || other.isEmpty | ||
| name.isEmpty || other.isEmpty || name == other |
There was a problem hiding this comment.
minor but alongside the other targetName change, noticed while profiling, this order seems to make more sense to avoid calling into string equality if we can
| try new String(file.toByteArray, codec.charSet).toCharArray | ||
| catch case _: FileSystemException => Array.empty[Char] | ||
| try new String(file.toByteArray, codec.charSet) | ||
| catch case _: FileSystemException => "" |
There was a problem hiding this comment.
this will disappear in #26512 to call into the filesystem directly to read a string, hopefully avoiding this copy
| def visitInternalName(internalName: String, offset: Int, length: Int): Unit = if (internalName != null && containsChar(internalName, offset, length, '$')) { | ||
| for (c <- getClassIfNested(internalName.substring(offset, length))) | ||
| def visitInternalName(internalName: String, beginIndex: Int, endIndex: Int): Unit = if (internalName != null && containsChar(internalName, beginIndex, endIndex, '$')) { | ||
| for (c <- getClassIfNested(internalName.substring(beginIndex, endIndex))) |
There was a problem hiding this comment.
substring takes beginIndex, endIndex so the parameter names to this function were wrong, though it was called with the right values; and that means containsChar was wrong, though this seems to not have caused problems.
| val start = i + 1 // skip the L | ||
| var seenDollar = false | ||
| while ({val ch = desc.charAt(i); seenDollar ||= (ch == '$'); ch != ';'}) i += 1 | ||
| if (seenDollar) |
There was a problem hiding this comment.
the callee already checks for this so this check isn't needed
|
Benchmarks started. Workflow run. |
|
Benchmarks completed. Overview. |
|
Benchmarks started. Workflow run. |
|
Benchmarks completed. Overview. |
4 participants
Depends on #26826
Hopefully we can avoid copies + benefit from the JVM's compact string optimization. Also methods like
indexOfno longer box.0-5% speedup, see the last benchmark results in this PR.
Have you relied on LLM-based tools in this contribution?
No
How was the solution tested?
Covered by existing tests (this is a refactoring)