[Runtime/ABI] Replace `emitTypeFieldAccessor` with a single runtime method #13926

xedin · 2018-01-13T06:41:52Z

Emitting individual accessors per field for each nominal type is inefficient from code size
and general code reusability perspective, because that duplicates data which already
exists in reflection section of the binary. This PR is a series of changes to get rid of
special lazy accessors and replace them with a single runtime method to request type field
metadata.

Resolves: rdar://problem/26060144

xedin · 2018-01-13T06:44:28Z

@DougGregor @jckarter @slavapestov Can you please take a look if this is a a correct initial direction?

slavapestov · 2018-01-13T07:00:46Z

include/swift/Runtime/RuntimeFunctions.def

@@ -1222,6 +1222,14 @@ FUNCTION(RegisterTypeMetadataRecords,
         ARGS(TypeMetadataRecordPtrTy, TypeMetadataRecordPtrTy),
         ATTRS(NoUnwind))

+// void swift_registerTypeFields(const FieldDescriptor *begin,
+//                               const FieldDescriptor *end)
+FUNCTION(RegisterTypeFields,


This file is only used for functions called from IRGen, so I don't think this is necessary.

Gotcha! Will remove.

slavapestov · 2018-01-13T07:01:24Z

stdlib/public/runtime/MetadataLookup.cpp

+
+struct TypeFieldMetadataState {
+  ConcurrentMap<DescriptorCacheEntry<FieldDescriptor>> FieldCache;
+  std::vector<FieldMetadataSection> SectionsToScan;


Are we allowed to use std::vector from the runtime?

That's what the rest of the caches in this file are doing too - protocols and type metadata, is that incorrect?

std::vector is okay in the runtime.

slavapestov · 2018-01-13T07:01:49Z

This looks like a good start, I'm looking forward to this coming together!

xedin · 2018-01-13T10:41:30Z

I've started using swift_getTypeByMangledName but I have no idea what were to get arguments for it expect for the mangled name, is that ok?

Also I've noticed that reflection field record doesn't have "weak" in flags, is that intentional?

DougGregor

Definitely looks like the right approach!

DougGregor · 2018-01-14T06:55:19Z

stdlib/public/runtime/MetadataLookup.cpp

+    auto type = field.getMangledTypeName(0);
+
+    auto *typeMetadata = swift_getTypeByMangledName(type.data(), type.length(),
+                                                    0, nullptr, nullptr);


Until we're able to provide the substitutions to swift_getTypeByMangledName, this won't work for generic types. There's no good way to get all of those substitutions correctly now (the information isn't there in the nominal type descriptors), but you should be able to handle non-nested generics with no requirements by using TargetMetadata::getGenericArgs(). That would at least be enough to see basic substitutions working.

Fully supporting substitutions here will require @jckarter to land #13468

Good to know, thanks, @DougGregor ! In a state I have it right now, would it be able to find non-generic types? Also, all of the methods in Field{Descriptor, Record} require Offset as an argument, I looked at how swift-reflection-dump gets it, but I don't think it's possible to do the same in ImageInspection{ELF, MachO} because we there is no SectionRef, is there some other way to get it without using object file interface?

Talked to @jckarter offline and it looks like I don't need to worry about offset I mentioned in my previous comment. So the only thing which remains is to get some information from given type to supply generic env to the swift_getTypeByMangledName call and wait for other changes to be landed.

xedin · 2018-01-17T09:05:44Z

stdlib/public/runtime/MetadataLookup.cpp

+
+    auto *typeMetadata =
+        swift_getTypeByMangledName(type.data(), type.length(), numberOfLevels,
+                                   paramsPerLevel, base->getGenericArgs());


@DougGregor Is this the correct way of using getGenericArgs? I've also modified interface a bit to use a callback instead of trying to return std::pair, WDYT?

Since you're using a callback, you might also be able to make the callback use StringRef instead of std::string, since it could have access to memory from swift_getFieldAt's stack frame.

xedin · 2018-01-17T23:09:46Z

@swift-ci please smoke test

xedin · 2018-01-18T08:29:02Z

Ok, I'm getting closer, only 9 stdlib tests now fail locally and I think they all related to generics, I'll continue tomorrow.

xedin · 2018-01-18T20:53:56Z

@DougGregor @jckarter I got this to the point where only failing tests are related to unimplemented things in DecodedMetadataBuilder like weak storage.

xedin · 2018-01-19T01:44:25Z

Only remaining problems in tests are related to REPL, so I'm trying to figure out how to register new types there. It seems like newly defined types are not picked up automatically via image inspection callbacks.

jckarter · 2018-01-19T17:55:09Z

@xedin Awesome! In JIT mode, we need to manually emit a call that registers the data from the newly-generated code since there's no "image" per se. Look at IRGenModule::emitRuntimeRegistration(), which generates runtime calls to manually register generated code with the ObjC and Swift runtimes in JIT mode.

xedin · 2018-01-20T22:14:50Z

@DougGregor dynamic registration now works! I'm waiting for #14038 to go in before all of the tests are green (I think), because it fails to demangle some imported types in function counter tests.

xedin · 2018-01-21T00:36:59Z

Ok I verified that we can't produce metadata for 'Builtin.RawPointer' and other 'Builtin' types right now, that's why function counter tests are failing.

slavapestov · 2018-01-21T06:18:08Z

This is looking good! Don't forget to remove the old mechanism, too.

xedin · 2018-01-21T08:31:00Z

Thanks, @slavapestov! Will remove the old code once tests are all green! :)

xedin · 2018-01-22T04:27:18Z

In my local build the biggest difference is libSwiftCore (which I think should be stdlib with everything else?), it's almost 64KB smaller than before.

xedin · 2018-02-21T02:05:28Z

Ok, looks like I have to rebase changes to type context descriptors and re-run everything again :)

Use information from reflection section of the binary to lookup type field info such as name and it's type and return it using new `swift_getFieldAt` method based on nominal type and field index.

One of my previous commits did that, but since we have Builtins reflection section we don't really want to generate reflection field metadata for opaque structs/enums. This reverts to the previous behavior.

…mework

…therNominalTypes when demangling bound generic types.

Since Mirrors are using swift_getFieldAt we need built-in type metadata present in JIT/REPL mode as well, otherwise some of the types are going to become unprintable.

xedin · 2018-02-21T02:22:28Z

What I really meant is 😭

All of their usages have been replaced with new runtime `swift_getFieldAt` method.

All of the information contained by this field (list of property names) is already encoded as part of the field reflection metadata and is accessible via `swift_getFieldAt` runtime method.

xedin · 2018-02-21T03:06:08Z

@swift-ci please test

xedin · 2018-02-21T03:06:21Z

@swift-ci please test source compatibility

xedin · 2018-02-21T05:11:11Z

@swift-ci please smoke test compiler performance

xedin · 2018-02-21T05:33:22Z

@swift-ci please smoke test compiler performance

swift-ci · 2018-02-21T06:17:50Z

Build comment file:

Summary for master smoketest

Unexpected test results, stats may be off for 3

Regressions found (see below)

Debug
- debug brief
- debug detailed
Release
- release brief
- release detailed

Debug

debug brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (0)

name	old	new	delta	delta_pct

Unchanged (delta < 1.0% or delta < 100.0ms) (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	43,595,272	43,217,768	-377,504	-0.87%
time.swift-driver.wall	60.6s	60.5s	-69.5ms	-0.11%

debug detailed

Regressed (0)

name	old	new	delta	delta_pct

Improved (4)

name	old	new	delta	delta_pct
IRModule.NumIRBasicBlocks	109,850	107,531	-2,319	-2.11% ✅
IRModule.NumIRFunctions	62,830	61,942	-888	-1.41% ✅
IRModule.NumIRGlobals	82,747	81,659	-1,088	-1.31% ✅
IRModule.NumIRValueSymbols	126,250	124,797	-1,453	-1.15% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (19)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	71,295	71,200	-95	-0.13%
AST.NumLoadedModules	10,859	10,859	0	0.0%
AST.NumTotalClangImportedEntities	198,218	198,016	-202	-0.1%
AST.NumUsedConformances	4,190	4,217	27	0.64%
IRModule.NumIRInsts	1,282,471	1,270,158	-12,313	-0.96%
LLVM.NumLLVMBytesOutput	43,595,272	43,217,768	-377,504	-0.87%
SILModule.NumSILGenFunctions	76,944	76,929	-15	-0.02%
SILModule.NumSILOptFunctions	53,309	53,473	164	0.31%
Sema.NumConformancesDeserialized	181,726	183,020	1,294	0.71%
Sema.NumConstraintScopes	478,216	479,774	1,558	0.33%
Sema.NumDeclsDeserialized	1,381,274	1,386,416	5,142	0.37%
Sema.NumDeclsValidated	44,937	45,069	132	0.29%
Sema.NumFunctionsTypechecked	41,790	41,707	-83	-0.2%
Sema.NumGenericSignatureBuilders	64,307	64,313	6	0.01%
Sema.NumLazyGenericEnvironments	249,939	250,795	856	0.34%
Sema.NumLazyGenericEnvironmentsLoaded	29,405	29,453	48	0.16%
Sema.NumLazyIterableDeclContexts	241,143	241,486	343	0.14%
Sema.NumTypesDeserialized	1,437,781	1,443,195	5,414	0.38%
Sema.NumTypesValidated	188,689	188,777	88	0.05%

Release

release brief

Regressed (0)

name	old	new	delta	delta_pct

Improved (2)

name	old	new	delta	delta_pct
LLVM.NumLLVMBytesOutput	40,191,936	39,628,372	-563,564	-1.4% ✅
time.swift-driver.wall	100.9s	99.5s	-1.4s	-1.42% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (0)

name	old	new	delta	delta_pct

release detailed

Regressed (12)

name	old	new	delta	delta_pct
AST.NumImportedExternalDefinitions	9,945	10,387	442	4.44% ⛔
AST.NumLoadedModules	354	365	11	3.11% ⛔
AST.NumTotalClangImportedEntities	28,853	29,955	1,102	3.82% ⛔
AST.NumUsedConformances	4,117	4,242	125	3.04% ⛔
Sema.NumDeclsDeserialized	197,005	200,549	3,544	1.8% ⛔
Sema.NumDeclsValidated	28,000	28,395	395	1.41% ⛔
Sema.NumFunctionsTypechecked	10,941	11,257	316	2.89% ⛔
Sema.NumGenericSignatureBuilders	7,495	7,694	199	2.66% ⛔
Sema.NumLazyGenericEnvironments	32,153	32,787	634	1.97% ⛔
Sema.NumLazyGenericEnvironmentsLoaded	3,804	3,871	67	1.76% ⛔
Sema.NumLazyIterableDeclContexts	20,178	20,706	528	2.62% ⛔
Sema.NumTypesDeserialized	229,504	233,346	3,842	1.67% ⛔

Improved (6)

name	old	new	delta	delta_pct
IRModule.NumIRBasicBlocks	88,996	84,799	-4,197	-4.72% ✅
IRModule.NumIRFunctions	40,431	39,202	-1,229	-3.04% ✅
IRModule.NumIRGlobals	47,479	45,833	-1,646	-3.47% ✅
IRModule.NumIRInsts	860,772	839,264	-21,508	-2.5% ✅
IRModule.NumIRValueSymbols	83,059	80,705	-2,354	-2.83% ✅
LLVM.NumLLVMBytesOutput	40,191,936	39,628,372	-563,564	-1.4% ✅

Unchanged (delta < 1.0% or delta < 100.0ms) (5)

name	old	new	delta	delta_pct
SILModule.NumSILGenFunctions	21,816	21,816	0	0.0%
SILModule.NumSILOptFunctions	28,245	28,245	0	0.0%
Sema.NumConformancesDeserialized	94,094	94,730	636	0.68%
Sema.NumConstraintScopes	446,681	449,763	3,082	0.69%
Sema.NumTypesValidated	56,394	56,801	407	0.72%

xedin · 2018-02-21T06:51:33Z

Looks like we save about half a megabyte with this changes

jckarter · 2018-02-21T18:47:14Z

stdlib/public/runtime/MetadataLookup.cpp

+    getFieldAt(*Value->getDescription());
+    return;
+  }
+  */


Do you want to try turning the cache on before committing, or do that in a followup?

I think it could be done in the follow up, I'm still trying to figure out what is safe to cache and what is not...

jckarter · 2018-02-21T18:48:26Z

Looks good! It's probably a good idea to land this and then improve the layout, enable the cache, etc. in follow-up commits.

xedin · 2018-02-21T18:49:41Z

@jckarter Thanks! I'll land it and do couple of improvements later then!

bitjammer · 2018-04-27T17:07:35Z

I just came here to say:

THIS IS SUPERB.

xedin requested review from DougGregor, jckarter and slavapestov January 13, 2018 06:43

xedin force-pushed the rdar-26060144 branch from 6df5de5 to 1d8727f Compare January 13, 2018 06:43

xedin force-pushed the rdar-26060144 branch from 1d8727f to a78c18c Compare January 13, 2018 06:45

slavapestov reviewed Jan 13, 2018

View reviewed changes

xedin force-pushed the rdar-26060144 branch from a78c18c to d0eedca Compare January 13, 2018 07:18

DougGregor reviewed Jan 14, 2018

View reviewed changes

xedin force-pushed the rdar-26060144 branch from ea03395 to 5eb80b6 Compare January 17, 2018 09:04

xedin commented Jan 17, 2018

View reviewed changes

xedin force-pushed the rdar-26060144 branch from 5eb80b6 to 9fdfe67 Compare January 17, 2018 23:08

xedin force-pushed the rdar-26060144 branch from 9fdfe67 to 47d200a Compare January 18, 2018 08:28

xedin force-pushed the rdar-26060144 branch from 47d200a to 5118dae Compare January 18, 2018 20:52

xedin force-pushed the rdar-26060144 branch from f8f0540 to b75bc54 Compare January 20, 2018 22:12

xedin force-pushed the rdar-26060144 branch from b75bc54 to 4051b57 Compare January 21, 2018 00:35

xedin force-pushed the rdar-26060144 branch from e4a0d80 to 316114d Compare January 22, 2018 06:28

xedin and others added 12 commits February 20, 2018 18:18

[Runtime] Register type field metadata section

bc1c170

[Runtime/Reflection] Add swift_getFieldAt runtime accessor

0da5e48

Use information from reflection section of the binary to lookup type field info such as name and it's type and return it using new `swift_getFieldAt` method based on nominal type and field index.

[Runtime/ABI] Add ownership info to the name-to-type metadata builder

c44ad57

[Runtime/Metadata] Add support for dynamic field descriptor registration

877c70b

[Runtime/Reflection] Switch enum mirrors to use runtime get field method

7a7825f

[Runtime] NFC: Update names of the field section to swift5_fieldmd

50785df

[IRGen] Don't generate field reflection metadata for opaque struct/enum

8bc7ba2

One of my previous commits did that, but since we have Builtins reflection section we don't really want to generate reflection field metadata for opaque structs/enums. This reverts to the previous behavior.

[Runtime/Metadata] Move swift_getFieldAt to new generic context fra…

7bcd730

…mework

[Metadata] Use ownership info from _typeByMangledName

3323ae3

[MetadataLookup] Properly setup Demangler symbolic ref resolver

194c84e

Demangler: Fill in places we need to handle symbolic references and O…

eb31681

…therNominalTypes when demangling bound generic types.

[IRGen/Reflection] Emit built-in reflection metadata in JIT mode

9d6dc72

Since Mirrors are using swift_getFieldAt we need built-in type metadata present in JIT/REPL mode as well, otherwise some of the types are going to become unprintable.

xedin added 3 commits February 20, 2018 18:45

[IRGen] Remove lazy field type accessor functions

f6be62d

All of their usages have been replaced with new runtime `swift_getFieldAt` method.

[IRGen] Remove 'FieldNames' field from type context descriptor

94017f7

All of the information contained by this field (list of property names) is already encoded as part of the field reflection metadata and is accessible via `swift_getFieldAt` runtime method.

[IRGen] Ensure that all of the field type metadata is properly forced

4370190

xedin force-pushed the rdar-26060144 branch from 758c399 to 4370190 Compare February 21, 2018 03:05

swiftlang deleted a comment from swift-ci Feb 21, 2018

jckarter reviewed Feb 21, 2018

View reviewed changes

xedin merged commit c8e3724 into swiftlang:master Feb 21, 2018

[Runtime/ABI] Replace emitTypeFieldAccessor with a single runtime method #13926

[Runtime/ABI] Replace emitTypeFieldAccessor with a single runtime method #13926

Uh oh!

Conversation

xedin commented Jan 13, 2018

Uh oh!

xedin commented Jan 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Jan 13, 2018

Uh oh!

xedin commented Jan 13, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Jan 17, 2018

Uh oh!

xedin commented Jan 18, 2018

Uh oh!

xedin commented Jan 18, 2018

Uh oh!

xedin commented Jan 19, 2018

Uh oh!

jckarter commented Jan 19, 2018

Uh oh!

xedin commented Jan 20, 2018

Uh oh!

xedin commented Jan 21, 2018

Uh oh!

slavapestov commented Jan 21, 2018

Uh oh!

xedin commented Jan 21, 2018

Uh oh!

xedin commented Jan 22, 2018

Uh oh!

xedin commented Feb 21, 2018

Uh oh!

xedin commented Feb 21, 2018

Uh oh!

xedin commented Feb 21, 2018

Uh oh!

xedin commented Feb 21, 2018

Uh oh!

xedin commented Feb 21, 2018

Uh oh!

xedin commented Feb 21, 2018

Uh oh!

swift-ci commented Feb 21, 2018

Build comment file:

Summary for master smoketest

Debug

debug brief

debug detailed

Release

release brief

release detailed

Uh oh!

xedin commented Feb 21, 2018

[Runtime/ABI] Replace `emitTypeFieldAccessor` with a single runtime method #13926

[Runtime/ABI] Replace `emitTypeFieldAccessor` with a single runtime method #13926