Skip to content

use_string_buffers reports, although intermediate strings are used #57777

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Hixie opened this issue Aug 18, 2018 · 3 comments
Closed

use_string_buffers reports, although intermediate strings are used #57777

Hixie opened this issue Aug 18, 2018 · 3 comments
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@Hixie
Copy link
Contributor

Hixie commented Aug 18, 2018

The first block in the following pattern triggers use_string_buffers but the second block (which uses StringBuffer) is ~7% slower:

const int count = 100000;

void test(String s) { }

void main() {
  Stopwatch t = new Stopwatch();

  // USING STRING
  t.start();
  String s = '';
  for (int i = 0; i < count; i += 1) {
    s = s + '-$i';
    test(s);
  }
  print(t.elapsedMicroseconds);

  // USING STRINGBUFFER
  t.reset();
  StringBuffer b = new StringBuffer();
  for (int i = 0; i < count; i += 1) {
    b.write('-$i');
    test(b.toString());
  }
  print(t.elapsedMicroseconds);
}
@lrhn
Copy link
Member

lrhn commented Aug 19, 2018

This is a tricky edge-case where a string is being built by repeated concatenation, the classical case where you should use StringBuffer, but where all the intermediate strings are also used. I'm not sure this isn't an acceptable false-positive for the hint, especially since performance isn't really an issue.

The benchmark isn't very precise. Try https://dartpad.dartlang.org/8515b26b4d4b4807d24bbe7cb36a8f91 instead - it runs each of these tests in a loop for two seconds, counting how many rounds it manages to do in that time, then repeats that three times to ensure all methods should be warm. It also adds versions that avoid creating the intermediate '-$i' string.

In JavaScript, there is no measurable difference in speed between the two versions (and a big advantage to versions that do not create the '-$o' intermediate string), but then, V8 does constant time concatenation, and we never do anything to the strings that forces flattening. If we change test to do s.indexOf('-'), then it gets 300 times slower.

When run on the VM, you need to lower count to 10000 or it won't even do one round in two seconds on my home computer, and the StringBuffer versions are ~25% less efficient than the plain concatenation then. It gets better asymptotically, at count 100000, the StringBuffer versions are faster than the + concatenation.

@srawlins srawlins transferred this issue from dart-lang/sdk Jun 20, 2020
@pq pq added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) linter-false-positive labels Nov 6, 2020
@srawlins srawlins changed the title use_string_buffers false positives use_string_buffers reports, although intermediate strings are used Sep 3, 2022
@srawlins srawlins added the P3 A lower priority bug or feature request label Sep 22, 2022
@pq pq added P2 A bug or feature request we're likely to work on and removed P3 A lower priority bug or feature request labels Nov 15, 2022
@pq
Copy link
Member

pq commented Nov 15, 2022

Bumping to P2 since this one is a proposed canonical lint (dart-lang/core#784).

@srawlins
Copy link
Member

This is a duplicate request of #57619.

@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

5 participants