[SR-88] Fixing and reapplying stdlib mirror API changes #1058

austinzheng · 2016-01-23T03:58:47Z

This PR contains both a commit reverting the revert to the mirrors commit (original pull request here), as well as another commit containing fixes and changes. I took the opportunity to remove some additional code.

I'm working on running the test suite (unit and validation tests) on both OS X and Ubuntu, and will comment when they are done.

I do have two comments:

I'm a little surprised that the way generics work changed so drastically in the past few days that the code I had originally written stopped working. If this wasn't intentional, there might be a regression somewhere. I'll have to look through the commit history and try to pinpoint exactly what changed.
The original mirror for sets and collections cached a 'last accessed' index value and used that to speed up subsequent accesses if those accesses involved 'larger' indices. That optimization is gone now that all the collections in the stdlib use the same helper collection type to pass their children into a mirror initializer. I wanted to point that out in case it might matter.

Let me know if there are any questions. Thanks again!

austinzheng · 2016-01-23T03:59:18Z

I forgot to mention that I'll squash/rebase once I verify tests are working and get the go-ahead.

austinzheng · 2016-01-23T04:35:10Z

Tests pass on both OS X and Ubuntu.

Migrate mirrors to use CustomReflectable API, rewrite dump() ...and there was much rejoicing.

dabrahams · 2016-01-23T16:54:12Z

Did you intend to squash these?

austinzheng · 2016-01-23T17:55:16Z

Yeah, eventually. Wanted to give people the chance to look at the changes first.

Austin

On Jan 23, 2016, at 8:55 AM, Dave Abrahams [email protected] wrote:

Did you intend to squash these?

—
Reply to this email directly or view it on GitHub #1058 (comment).

dabrahams · 2016-01-23T18:26:42Z

on Fri Jan 22 2016, Austin Zheng <notifications-AT-i.8713187.xyz> wrote:

The original mirror for sets and collections cached a 'last
accessed' index value and used that to speed up subsequent accesses if
those accesses involved 'larger' indices. That optimization is gone
now that all the collections in the stdlib use the same helper
collection type to pass their children into a mirror initializer. I
wanted to point that out in case it might matter.

@egranata, are you still relying on this optimization?

-Dave

dabrahams · 2016-01-23T18:36:17Z

on Fri Jan 22 2016, Austin Zheng <notifications-AT-i.8713187.xyz> wrote:

I'm a little surprised that the way generics work changed so
drastically in the past few days that the code I had originally
written stopped working. If this wasn't intentional, there might be a
regression somewhere. I'll have to look through the commit history and
try to pinpoint exactly what changed.

I must be missing something. I don't see anything in your fixes that
look like they point to a change in the way generics work. Can you
explain?

-Dave

austinzheng · 2016-01-23T18:45:47Z

Sure. What I meant to say was that the code in my original PR was building on my machine because it was based on a commit that was about 50 commits (a few day's worth) behind where master actually was at that time. When I updated my local master to match upstream's master, and then rebased my PR branch onto the updated master, only then did it fail to build correctly for me. Because of this I concluded that at least one of those ~50 changes between the two master commits contained something that made my original PR code stop working. Since the error in question was a type error involving generics, I imagine one of three things happened:

There was an incorrect use of generics in the code in my PR branch that wasn't being caught before, but a compiler bug was fixed and that incorrect use started to be caught.
One of those 50 commits introduced a regression, and my correct use of generics was incorrectly flagged as being an error.
One of those 50 commits intentionally changed how generics worked, and my previously-valid code now became invalid.

Let me know if that was any clearer. Thanks!

Austin

On Jan 23, 2016, at 10:37 AM, Dave Abrahams [email protected] wrote:

on Fri Jan 22 2016, Austin Zheng <notifications-AT-i.8713187.xyz> wrote:

I'm a little surprised that the way generics work changed so
drastically in the past few days that the code I had originally
written stopped working. If this wasn't intentional, there might be a
regression somewhere. I'll have to look through the commit history and
try to pinpoint exactly what changed.

I must be missing something. I don't see anything in your fixes that
look like they point to a change in the way generics work. Can you
explain?

-Dave
—
Reply to this email directly or view it on GitHub #1058 (comment).

dabrahams · 2016-01-23T21:32:55Z

It’s a little clearer, but letting us look at the un-squashed commits is only useful for us in tracking that down if you separate the changes that actually fixed breakage from the other stuff. And my point is, looking at ed69418, I can’t tell what made the difference.

austinzheng · 2016-01-23T21:38:16Z

Okay, that makes sense. I'll squash the commits this afternoon.

The breakage-fixing 'change' was changing the Mirrors to not use the helper collections that were deleted in that commit. Sorry for not making that clearer.

Sent from my iPhone

On Jan 23, 2016, at 1:34 PM, Dave Abrahams [email protected] wrote:

It’s a little clearer, but letting us look at the un-squashed commits is only useful for us in tracking that down if you separate the changes that actually fixed breakage from the other stuff. And my point is, looking at ed69418, I can’t tell what made the difference.

—
Reply to this email directly or view it on GitHub.

austinzheng · 2016-01-26T03:03:43Z

I just wanted to know if there's anything I can do to help push this forward. If not, I can ask again in a few days.

gribozavr · 2016-01-26T04:25:40Z

@austinzheng Could you resolve conflicts with the master branch?

austinzheng · 2016-01-26T04:28:03Z

I'll do that right now; however, I think the points @dabrahams still need to be addressed. Let me know if you have questions.

On Jan 25, 2016, at 8:26 PM, Dmitri Gribenko [email protected] wrote:

@austinzheng https://github.com/austinzheng Could you resolve conflicts with the master branch?

—
Reply to this email directly or view it on GitHub #1058 (comment).

gribozavr · 2016-01-26T04:31:18Z

Yes, we still need to decide what to do with that optimization in Dictionary's mirror. I know it is important for the debugger, I want to read the new code and consider how it would perform.

austinzheng · 2016-01-26T04:37:03Z

I squashed and rebased onto latest master. I'm running tests right now just to verify nothing is broken. Once you've decided what you want to do with Dictionary's mirror, let me know.

Also, as I mentioned in one of the line comments(?), the collections now use the unlabeledChildren: constructor for Mirror, which means dump()ing them no longer displays the [0], [1], etc labels. If you don't like that I can bring back the old behavior.

austinzheng · 2016-01-26T05:10:48Z

As of now, all tests on this branch pass on both OS X and Ubuntu.

gribozavr · 2016-01-26T21:22:55Z

stdlib/public/SDK/AppKit/AppKit.swift

+      return .View(NSImage())
+    } else {
+      _NSViewQuickLookState.views.insert(self)
+      let result : PlaygroundQuickLook


We don't put a space before the colon in variable declarations.

Changes: - Reverted commit reverting original SR-88 commit - Removed mirror children helper collections and related code - Rewrote some tests to keep them working properly - Wrote two more tests for the three pointer APIs to ensure no crashes if created using a value > Int64.max This reverts commit 8917eb0.

austinzheng · 2016-01-27T03:52:19Z

All tests now pass on OS X. Linux pending...

austinzheng · 2016-01-27T04:15:27Z

Unfortunately I have to head out now, but if you trigger the CI to run on Linux it should almost certainly pass. Let me know if you have any questions.

shahmishal · 2016-01-27T04:22:03Z

@swift-ci Please test Linux platform

gribozavr · 2016-01-27T05:02:35Z

@swift-ci Please test Linux platform

gribozavr · 2016-01-27T06:46:27Z

@austinzheng This is amazing work, thank you!

@swift-ci 👍

dabrahams · 2016-01-27T07:13:42Z

Yes, @austinzheng, thank you again for taking this on!

austinzheng · 2016-01-27T07:50:49Z

I'm glad it's finally in! Fingers crossed that it won't break anything 😬.

This pull request broke the following tests on several build configurations (eg --preset=buildbot,tools=RA,stdlib=DA) 1_stdlib/Reflection.swift 1_stdlib/ReflectionHashing.swift 1_stdlib/UnsafePointer.swift.gyb This reverts commit c223a3b, reversing changes made to 5c2bb09.

atrick · 2016-01-27T18:56:39Z

It looks like the reflection tests are breaking in the asserts build. You can revert my revert commit below and provide a fix on top of that.

commit 182bb7f (HEAD -> master, public/master, public/HEAD)
Author: Andrew Trick [email protected]
Date: Wed Jan 27 10:43:08 2016

Revert "Merge pull request #1058 from austinzheng/az-port-mirror"

This pull request broke the following tests on several build configurations
(eg --preset=buildbot,tools=RA,stdlib=DA)

    1_stdlib/Reflection.swift
    1_stdlib/ReflectionHashing.swift
    1_stdlib/UnsafePointer.swift.gyb

This reverts commit c223a3bf06e3d4173f0d6cf0ad21c41d344ca1e0, reversing
changes made to 5c2bb09b092ffd23e2afc8e31a0a7f2f93974aa1.

atrick · 2016-01-27T19:01:01Z

The key to reproducing the failures is probably --swift-stdlib-build-type=Debug --swift-stdlib-enable-assertions=true

austinzheng · 2016-01-27T19:07:32Z

Thanks for bringing this to my attention; I'll take a look tonight. Are
those arguments to build-script?

Are there any other configs I can test against before submitting a fix?

Austin

On Wed, Jan 27, 2016 at 11:02 AM, atrick [email protected] wrote:

The key to reproducing the failures is probably
--swift-stdlib-build-type=Debug --swift-stdlib-enable-assertions=true

—
Reply to this email directly or view it on GitHub
#1058 (comment).

atrick · 2016-01-27T19:19:29Z

There were two kinds of failures that persisted after this commit:

stdlib asserts build

Both the release and debug stdlib build with asserts have the same failures:

build-script --preset=buildbot,tools=RA,stdlib=DA
build-script --preset=buildbot,tools=RA,stdlib=RDA

I'm guessing the relevant build-script option is --swift-stdlib-enable-assertions=true

1_stdlib/Reflection.swift
1_stdlib/ReflectionHashing.swift
1_stdlib/UnsafePointer.swift.gyb

LLDB: both OSX and Ubuntu

build-script --debug-swift --debug-lldb --test -- --skip-test-cmark --skip-test-swift --lldb-use-system-debugserver

Timeout in TestREPLQuickLookObject.REPLQuickLookTestCase.testREPL

It won't be clear whether this is related until my revert passes, but it seems likely.

austinzheng · 2016-01-27T19:58:43Z

Hi @atrick,

I can't build the project using the --preset=buildbot config presented above; it complains about not having some other variables set. I'm trying to reproduce with ./utils/build-script --swift-stdlib-assertions --swift-assertions --debug-swift --debug-swift-stdlib --test; do you think this is sufficient?

atrick · 2016-01-27T20:01:22Z

That should be sufficient, but if you can just define some dummy variables like this:

build-script install_destdir=$install_dir installable_package=$build_name.tar.gz

austinzheng · 2016-01-27T20:31:34Z

Hi @atrick,

I checked out the latest master, updated my dependencies, backed out your commit, and ran the tests on my machine using the following command:

./utils/build-script --swift-stdlib-assertions --swift-assertions --debug-swift --debug-swift-stdlib --clean --test.

However, the unit test suite appears to complete successfully for me. (OS X 10.11.1, MBP 15")

I'm going to try defining the dummy vars and running the preset.

atrick · 2016-01-27T21:44:18Z

The linux test failure, Timeout in TestREPLQuickLookObject.REPLQuickLookTestCase.testREPL, went away after reverting this pull request.

It will take the bots a long time to verify that the Reflection tests are fixed. Meanwhile I'm also trying to reproduce them locally.

austinzheng · 2016-01-27T21:49:57Z

@atrick Is this failure only on Linux? Do you know if it passed on OS X?

gribozavr · 2016-01-27T22:01:28Z

I think the failure was on iphonesimulator-i386. You can run tests with ninja check-swift-iphonesimulator-i386 in the build directory.

austinzheng · 2016-01-27T22:07:44Z

Excellent, I'm running the test suite now. I wonder if my "bigger than
Int64.max" test broke something on that architecture.

For future reference, what's the maximal set of tests I should run before
filing another PR? Should I run the check-swift command against all the
Apple architectures in the build directory?

Thanks,
Austin

On Wed, Jan 27, 2016 at 2:02 PM, Dmitri Gribenko [email protected]
wrote:

I think the failure was on iphonesimulator-i386. You can run tests with ninja
check-swift-iphonesimulator-i386 in the build directory.

—
Reply to this email directly or view it on GitHub
#1058 (comment).

gribozavr · 2016-01-27T22:11:27Z

Usually build-script -RT is sufficient. Sometimes a pull request can change some platform-dependent behavior, like i386 simulator, but it is hard to tell in advance. You can run tests for extra platforms if you want to be extra safe, but we can't require that for practical reasons.

austinzheng · 2016-01-27T22:16:19Z

Alright, that's super helpful.

I see the i386 breaking behavior; I can fix it when I get back tonight.
However, not sure about the timeout Linux issue; I'll have to dig into that
further.

One question for now: I thought UInt and Int were always 64-bit, as per the
stdlib documentation. However, it seems like they are variable width like
the equivalent C types, hence "error: integer overflows when converted from
'UInt64' to 'UInt'". Is that true, or documented anywhere?

Austin

On Wed, Jan 27, 2016 at 2:12 PM, Dmitri Gribenko [email protected]
wrote:

Usually build-script -RT is sufficient. Sometimes a pull request can
change some platform-dependent behavior, like i386 simulator, but it is
hard to tell in advance. You can run tests for extra platforms if you want
to be extra safe, but we can't require that for practical reasons.

—
Reply to this email directly or view it on GitHub
#1058 (comment).

gribozavr · 2016-01-27T22:19:07Z

Thanks!

However, it seems like they are variable width like the equivalent C types

Yes, that's correct -- Int and UInt are of the same size as the platform word (32 bits on i386 iphonesimulator).

atrick · 2016-01-27T22:36:12Z

Sorry, the LLDB failure is not linux specific. The timeout on TestREPLQuickLookObject reproduces on OSX.

build-script --debug-swift --debug-lldb --test -- --skip-test-cmark --skip-test-swift --lldb-use-system-debugserver

=== BUILD TARGET repl_swift OF PROJECT lldb WITH CONFIGURATION CustomSwift-Debug ===

Timeout in TestREPLQuickLookObject.REPLQuickLookTestCase.testREPL

@scallanan I haven't reproduced this myself yet. If you have trouble, maybe someone from lldb can help.

austinzheng force-pushed the az-port-mirror branch from 7167c34 to 88b828b Compare January 23, 2016 04:01

austinzheng force-pushed the az-port-mirror branch from 88b828b to c8e6b50 Compare January 23, 2016 04:43

austinzheng referenced this pull request Jan 23, 2016

Merge pull request #838 from austinzheng/az-port-mirror

d72d808

Migrate mirrors to use CustomReflectable API, rewrite dump() ...and there was much rejoicing.

austinzheng force-pushed the az-port-mirror branch from c8e6b50 to 8d868da Compare January 23, 2016 19:53

austinzheng force-pushed the az-port-mirror branch from 8d868da to d5d66c7 Compare January 26, 2016 04:33

gribozavr reviewed Jan 26, 2016
View reviewed changes

austinzheng force-pushed the az-port-mirror branch from a4114e2 to 9ab9d4f Compare January 27, 2016 03:21

austinzheng force-pushed the az-port-mirror branch from 9ab9d4f to 10d5b23 Compare January 27, 2016 03:28

gribozavr self-assigned this Jan 27, 2016

swift-ci added a commit that referenced this pull request Jan 27, 2016

Merge pull request #1058 from austinzheng/az-port-mirror

c223a3b

swift-ci merged commit c223a3b into swiftlang:master Jan 27, 2016

austinzheng deleted the az-port-mirror branch June 8, 2016 02:46

[SR-88] Fixing and reapplying stdlib mirror API changes #1058

[SR-88] Fixing and reapplying stdlib mirror API changes #1058

Uh oh!

Conversation

austinzheng commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 23, 2016

Uh oh!

dabrahams commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 23, 2016

Uh oh!

dabrahams commented Jan 23, 2016

Uh oh!

dabrahams commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 23, 2016

Uh oh!

dabrahams commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 23, 2016

Uh oh!

austinzheng commented Jan 26, 2016

Uh oh!

gribozavr commented Jan 26, 2016

Uh oh!

austinzheng commented Jan 26, 2016

Uh oh!

gribozavr commented Jan 26, 2016

Uh oh!

austinzheng commented Jan 26, 2016

Uh oh!

austinzheng commented Jan 26, 2016

Uh oh!

gribozavr Jan 26, 2016

Choose a reason for hiding this comment

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

shahmishal commented Jan 27, 2016

Uh oh!

gribozavr commented Jan 27, 2016

Uh oh!

gribozavr commented Jan 27, 2016

Uh oh!

dabrahams commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

atrick commented Jan 27, 2016

Uh oh!

atrick commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

atrick commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

atrick commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

atrick commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

gribozavr commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

gribozavr commented Jan 27, 2016

Uh oh!

austinzheng commented Jan 27, 2016

Uh oh!

gribozavr commented Jan 27, 2016