[benchmark] Add dataUsingUTF8Encoding() #22648

ianpartridge · 2019-02-15T11:12:16Z

Currently there is no microbench for string.data(using: .utf8).

However, there is a microbench which covers creating a Data from a String via Data(string.utf8).

It is worth having a separate bench for string.data(using: .utf8) because it currently bridges via NSString whereas Data(string.utf8) does not.

Add a benchmark for string.data(using: .utf8) so future improvements to its performance can be measured.

CC: @milseman @itaiferber @Catfish-Man

benchmark/single-source/DataBenchmarks.swift

ianpartridge · 2019-02-15T20:26:18Z

@palimondo - I've updated as per feedback.

Please could someone run CI testing?

itaiferber · 2019-02-15T20:27:00Z

@swift-ci Please smoke test

itaiferber

Latest changes LGTM

palimondo · 2019-02-15T20:46:54Z

@swift-ci please benchmark

palimondo

Two more issues I've overlooked before:

new tests shouldn't use legacyFactor
benchmark reports shows there's memory leak, we should wrap that conversion in autoreleasepool

benchmark/single-source/DataBenchmarks.swift

palimondo · 2019-02-15T22:31:25Z

@milseman @itaiferber Any thoughts on the doubling of memory used between the runs with one and two iterations?
mem_pages [i1, i2]: min=[3299, 6662]

Is the use of @autoreleasepool required and expected, or is the "leak" a bug that needs to be reported?

itaiferber · 2019-02-15T22:40:22Z

@palimondo This isn't a real leak: the result of .data(using:) is an autoreleased NSData object. Absent an autoreleasepool around the construction of the data, it's going to remain present in memory until the outermost autoreleasepool releases it, which in this case is likely past the lifetime of the test.

So wrapping this call up is entirely reasonable

Co-Authored-By: ianpartridge <i.partridge@uk.ibm.com>

ianpartridge · 2019-02-18T09:38:07Z

@palimondo I have updated this PR as requested.

Please can someone re-run CI and merge as appropriate?

palimondo · 2019-02-18T09:40:26Z

@swift-ci please benchmark

palimondo · 2019-02-18T09:40:47Z

@swift-ci please smoke test

swift-ci · 2019-02-18T10:26:57Z

Performance: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
DataCreateEmpty	170	200	+17.6%	0.85x
DataCountSmall	22	25	+13.6%	0.88x
DataCountMedium	28	31	+10.7%	0.90x
Improvement
DataSubscriptSmall	31	28	-9.7%	1.11x (?)
FlattenListLoop	4332	3968	-8.4%	1.09x (?)
ObjectiveCBridgeStubFromNSDate	6190	5680	-8.2%	1.09x (?)
Array2D	7520	6912	-8.1%	1.09x
ObjectiveCBridgeStubNSDateRefAccess	400	371	-7.2%	1.08x (?)
FlattenListFlatMap	6557	6091	-7.1%	1.08x (?)
MapReduce	397	369	-7.1%	1.08x
MapReduceAnyCollection	398	370	-7.0%	1.08x
Added
String.data.Empty	91	100	94	—
String.data.LargeUnicode	537	548	542	—
String.data.Medium	435	438	437	—
String.data.Small	415	417	416	—

Code size: -O

TEST	OLD	NEW	DELTA	RATIO
Regression
DataBenchmarks.o	52228	54524	+4.4%	0.96x

Performance: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
SetSubtractingInt25	102	112	+9.8%	0.91x (?)
SetSubtractingInt0	74	80	+8.1%	0.93x (?)
Improvement
DataCreateSmall	3450	3110	-9.9%	1.11x (?)
DictionaryBridgeToObjC_Access	995	925	-7.0%	1.08x (?)
Added
String.data.Empty	87	89	88	—
String.data.LargeUnicode	541	550	544	—
String.data.Medium	439	440	439	—
String.data.Small	377	380	378	—

Code size: -Osize

TEST	OLD	NEW	DELTA	RATIO
Regression
DataBenchmarks.o	39764	41740	+5.0%	0.95x

Performance: -Onone

TEST	OLD	NEW	DELTA	RATIO
Regression
DataCreateEmpty	1080	1170	+8.3%	0.92x (?)
ObjectiveCBridgeStubFromNSString	941	1017	+8.1%	0.93x (?)
Added
String.data.Empty	107	110	108	—
String.data.LargeUnicode	564	581	570	—
String.data.Medium	479	483	481	—
String.data.Small	426	430	428	—

✅	Benchmark Check Report
⛔️⏱	`String.data.Medium` has setup overhead of 52 μs (12.7%). _{Move initialization of benchmark data to the setUpFunction registered in BenchmarkInfo.}

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

palimondo · 2019-02-18T12:27:04Z

@ianpartridge Thank you for the work on these benchmarks!

I'll merge as soon as somebody fixes the failing tests on linux (unrelated to this PR). We can ignore the ⛔️⏱ String.data.Medium. I have verified locally that there is no setup overhead... the benchmark validation heuristic can sometimes misfire, when there is high variability between independent runs, as is the case here:

ianpartridge · 2019-02-18T12:48:24Z

Thanks @palimondo, I hope we can merge soon.

palimondo · 2019-02-18T16:36:57Z

@swift-ci smoke test linux platform

milseman · 2019-02-18T18:20:46Z

@swift-ci please smoke test linux platform

palimondo · 2019-02-18T18:31:43Z

@milseman The Linux smoke test has been failing the whole weekend. This last one failed few minutes ago. Did somebody fix the CI yet? (it's not even accepting the requests via GitHub without excessive repetition...)

palimondo · 2019-02-18T18:52:26Z

@swift-ci smoke test linux platform

milseman · 2019-02-18T19:26:09Z

Yeah, @gottesmm says he's preparing a fix. Let's wait on that

palimondo · 2019-02-18T19:32:02Z

Oh, I've notices somebody kicked the CI servers and @swift-ci started to respond again, so I've restarted the smoke test hoping that was all it needed… Sorry for wasting the ⚡️🥺

milseman · 2019-02-18T19:47:03Z

#22692 I believe

palimondo · 2019-02-18T20:57:25Z

@swift-ci smoke test linux platform

#22692 was merged by Mishal, even though the test failed on Linux in that PR… let's see if that's it!

palimondo · 2019-02-18T22:11:12Z

Hmm… smoke test on Linux did successfully finish in #22673. Let's try again…

@swift-ci smoke test linux platform

palimondo · 2019-02-18T23:14:43Z

@milseman Can you please ask around why is the Linux smoke test still failing? This is just an addition of a few benchmarks… the failure is clearly unrelated to this PR! I need to get some 💤😴...

milseman · 2019-02-18T23:54:49Z

The failure is: TEST 'swift-package-tests :: build-swift-benchmarks-with-swiftpm.txt' FAILED, which could be related. The error message itself doesn't seem to be so we can try to clean or something. Other PRs are also failing, but for a different reason, so hold on a bit.

milseman · 2019-02-18T23:55:03Z

@swift-ci please clean smoke test linux platform

palimondo · 2019-02-19T05:43:50Z

OK, we are failing to build benchmarks on Linux with swiftpm: DataBenchmarks.swift:416:5: error: use of unresolved identifier 'autoreleasepool'…

Cross platform Swift sucks!

palimondo · 2019-02-19T06:55:59Z

@swift-ci please smoke test linux platform with #22707

Edit: Hmm… where are the docs for CI that tell me how to test 2 PR together?
I saw people use something like the above incantation, but it's not working for me ☹️🥺

Edit2: Aha, it had to be full URL in comment (GitHub shortens it for presentation)!
Continuous Integration: Cross Repository Testing

palimondo · 2019-02-19T07:08:29Z

Please smoke test with following pull request:
#22707

@swift-ci please smoke test linux platform

palimondo · 2019-02-19T16:18:20Z

For some reason the smoke test with combined PRs didn't work… The error on the console was the same. I guess we'll try again after the #22707 lands?

palimondo · 2019-02-20T11:56:01Z

@swift-ci please smoke test Linux platform

palimondo · 2019-02-20T14:37:42Z

Thank you @ianpartridge!

Currently, `str.data(using:allowLossyConversion)` always bridges via `NSString`. As `String` is now natively UTF8 we can fastpath this conversion in the case where the user requests UTF8 encoding. A benchmark for this was previously added in swiftlang#22648.

Currently, `str.data(using:allowLossyConversion)` always bridges via `NSString`. As `String` is now natively UTF8 we can fastpath this conversion in the case where the user requests UTF8 encoding. A benchmark for this was previously added in #22648.

[benchmark] Add StringToDataUsingUTF8Encoding

0fc4f34

ianpartridge force-pushed the benchmark-str2data branch from 090a8d3 to 0fc4f34 Compare February 15, 2019 12:24

parkera approved these changes Feb 15, 2019

View reviewed changes

itaiferber reviewed Feb 15, 2019

View reviewed changes

benchmark/single-source/DataBenchmarks.swift Outdated Show resolved Hide resolved

palimondo suggested changes Feb 15, 2019

View reviewed changes

benchmark/single-source/DataBenchmarks.swift Outdated Show resolved Hide resolved

Adjust benchmark naming

764f0cf

itaiferber approved these changes Feb 15, 2019

View reviewed changes

palimondo approved these changes Feb 15, 2019

View reviewed changes

ianpartridge changed the title ~~[benchmark] Add StringToDataUsingUTF8Encoding~~ [benchmark] Add dataUsingUTF8Encoding() Feb 15, 2019

This comment has been minimized.

Sign in to view

palimondo suggested changes Feb 15, 2019

View reviewed changes

benchmark/single-source/DataBenchmarks.swift Outdated Show resolved Hide resolved

benchmark/single-source/DataBenchmarks.swift Outdated Show resolved Hide resolved

palimondo and others added 2 commits February 18, 2019 09:34

Update benchmark/single-source/DataBenchmarks.swift

2b60780

Co-Authored-By: ianpartridge <i.partridge@uk.ibm.com>

Remove legacy factor

e9ba30a

palimondo approved these changes Feb 18, 2019

View reviewed changes

palimondo mentioned this pull request Feb 19, 2019

[benchmark] Move fake autoreleasepool to TestUtils #22707

Merged

palimondo merged commit 1cbc0b3 into swiftlang:master Feb 20, 2019

ianpartridge deleted the benchmark-str2data branch February 20, 2019 14:52

ianpartridge mentioned this pull request Apr 23, 2019

[String] [Foundation] perf: UTF8 String -> Data #24215

Merged

ianpartridge mentioned this pull request Apr 24, 2019

[5.1] [String] [Foundation] perf: UTF8 String -> Data #24239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[benchmark] Add dataUsingUTF8Encoding() #22648

[benchmark] Add dataUsingUTF8Encoding() #22648

ianpartridge commented Feb 15, 2019

ianpartridge commented Feb 15, 2019

itaiferber commented Feb 15, 2019

itaiferber left a comment

palimondo commented Feb 15, 2019

This comment has been minimized.

palimondo left a comment •

edited

Loading

palimondo commented Feb 15, 2019

itaiferber commented Feb 15, 2019

ianpartridge commented Feb 18, 2019

palimondo commented Feb 18, 2019

palimondo commented Feb 18, 2019

swift-ci commented Feb 18, 2019

palimondo commented Feb 18, 2019 •

edited

Loading

ianpartridge commented Feb 18, 2019

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 18, 2019 •

edited

Loading

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 18, 2019 •

edited

Loading

palimondo commented Feb 18, 2019

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 19, 2019 •

edited

Loading

palimondo commented Feb 19, 2019 •

edited

Loading

palimondo commented Feb 19, 2019

palimondo commented Feb 19, 2019

palimondo commented Feb 20, 2019

palimondo commented Feb 20, 2019

[benchmark] Add dataUsingUTF8Encoding() #22648

[benchmark] Add dataUsingUTF8Encoding() #22648

Conversation

ianpartridge commented Feb 15, 2019

ianpartridge commented Feb 15, 2019

itaiferber commented Feb 15, 2019

itaiferber left a comment

Choose a reason for hiding this comment

palimondo commented Feb 15, 2019

This comment has been minimized.

palimondo left a comment • edited Loading

Choose a reason for hiding this comment

palimondo commented Feb 15, 2019

itaiferber commented Feb 15, 2019

ianpartridge commented Feb 18, 2019

palimondo commented Feb 18, 2019

palimondo commented Feb 18, 2019

swift-ci commented Feb 18, 2019

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

palimondo commented Feb 18, 2019 • edited Loading

ianpartridge commented Feb 18, 2019

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 18, 2019 • edited Loading

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 18, 2019 • edited Loading

palimondo commented Feb 18, 2019

palimondo commented Feb 18, 2019

milseman commented Feb 18, 2019

milseman commented Feb 18, 2019

palimondo commented Feb 19, 2019 • edited Loading

palimondo commented Feb 19, 2019 • edited Loading

palimondo commented Feb 19, 2019

palimondo commented Feb 19, 2019

palimondo commented Feb 20, 2019

palimondo commented Feb 20, 2019

palimondo left a comment •

edited

Loading

palimondo commented Feb 18, 2019 •

edited

Loading

palimondo commented Feb 18, 2019 •

edited

Loading

palimondo commented Feb 18, 2019 •

edited

Loading

palimondo commented Feb 19, 2019 •

edited

Loading

palimondo commented Feb 19, 2019 •

edited

Loading