Cuckoo metadata by davidwrighton · Pull Request #24498 · dotnet/coreclr

davidwrighton · 2019-05-09T16:22:55Z

Replace attribute reading code with possibly higher performance path that uses a r2r generated cuckoo filter instead of searching CustomAttribute table.

Performance not yet fully investigated. Will finalize that before asking for review.

- Implement cuckoo filter lookup logic - Implement new ready to run section - Add dumper to R2RDump - Parse section on load into data structure - Implement function to query filter - Add concept of enum of well known attributes - So that attribute name hashes themselves may be cached

… with use of R2R data

This should fix the current error seen when compiling on Linux. The OSX issue with immintrin.h will be dealt with later.

tannergooding · 2019-05-09T20:38:39Z

Performance not yet fully investigated. Will finalize that before asking for review.

Are you going to use the nullable annotated S.P.Corelib as one of the tests?

tannergooding · 2019-05-09T20:42:11Z

+#ifndef __WELLKNOWNATTRIBUTES_H_
+#define __WELLKNOWNATTRIBUTES_H_
+
+enum class WellKnownAttribute : DWORD


This is only attributes that the VM/JIT cares about, correct?

At the moment. Yeah, its most of the ones the VM/JIT cares about. @sebastienros suggested I look into ASP.NET startup and see if I should see if I thought this might be useful to improve attribute query perf for managed code too.

davidwrighton · 2019-05-10T15:29:29Z

Performance not yet fully investigated. Will finalize that before asking for review.

Are you going to use the nullable annotated S.P.Corelib as one of the tests?

@tannergooding The tests I am looking at are small process start tests, which are primarily impacted by S.P.Corelib type load perf, so yeah, its the primary test vehicle. I need to find out if there is a threshold for which doing the hashing calculation is just more expensive than the table scan, but I suspect it will work quite well.

@tannergooding do you have a reference for the right way to detect if I can use ARM NEON instructions on Windows/Linux? With the current implementation, I'm not very optimistic about performance without the SIMD based comparison, so my preference would be to generate the optimized table unconditionally, but only use it if SIMD instructions are available for use.

tannergooding · 2019-05-10T15:40:10Z

do you have a reference for the right way to detect if I can use ARM NEON instructions on Windows/Linux?

We have some existing code in the VM utilizing IsProcessorFeaturePresent and some flags to try and detect this. However, we also just assume NEON/AdvSIMD is supported for ARM/ARM64.

You can see where we set the JIT flags and do other ISA checks for ARM/ARM64 here: https://github.com/dotnet/coreclr/blob/master/src/vm/codeman.cpp#L1481

It's also worth noting that Windows doesn't currently expose all the same ISA flags for ARM as Linux does, which goes through PAL_GetJitCpuCapabilityFlags instead.

…he feature

jkotas · 2019-05-10T17:37:14Z

With the current implementation, I'm not very optimistic about performance without the SIMD based comparison,

My guess is that the performance without SIMD would be just fine. The copy of the SIMD header looks like a maintenance nightmare...

davidwrighton · 2019-05-11T00:47:36Z

The SIMD header is deeply dissatisfying, however, I expect maintenance to be minimal. We've already been maintaining a very similar SIMD header for some SSE intrinsics in xmmintrin.h without much ongoing cost. I'll want to investigate the performance here before I make any judgements here though.

…reclr into cuckoo_metadata

- With current xor based second hash system, cuckoo filters must be powers of two. This can be addressed later, after performance is analyzed.

- Probably doesn't work on *nix platforms, but I'll discover that in the cloud

janvorli · 2019-05-15T15:58:07Z


+#ifndef DACCESS_COMPILE
+
+#if defined(_M_X64) || defined(__x86_64__) || defined(__i386__) || defined(_M_IX86)


Can you please use _TARGET_AMD64_ / _TARGET_X86_ / _TARGET_ARM_ / _TARGET_ARM64_ instead?

No, because these aren't about the target architecture. They are about the compilation architecture. I could not find a set of compilation architecture #defines to use.

Ah, right. Then you can use _ARM_, _ARM64_, _X86_ and _AMD64_

janvorli · 2019-05-15T16:00:41Z

+
+/*
+
+The below hash combining function is based on the xxHash32 logic implemented


It seems we should put the license into THIRD-PARTY-NOTICES.TXT too.

Its already in there. See License notice for xxHash.

- Our build environments aren't setup for NEON intrinsics - Performance win is affected by using non-SIMD path, but the cuckoo filter is still an overall win. Once this is all checked in, I'll open an issue to enable SIMD in our build.

…iler

- Fix the disable filter logic - Disable the presence table for non Corelib cases. Current performance data does not warrant the size increase in other generated binaries

davidwrighton · 2019-05-17T01:40:53Z

The performance improvement seems to be limited to coreLib, so I've restricted the implementation to be Corelib only to avoid excess bloat in R2R images. This is disappointing, but not surprising, as corelib has types loaded and referenced from it at rates which far exceed that of other dlls. As we put together more startup measurement data, we should revisit this policy.

…o_metadata Attempted to address conflict with native type parsing

jkotas · 2019-05-17T02:36:00Z

    }

+    // For format version 3.1 and later, there is an optional attributes section
+    if (IsImageVersionAtLeast(3,1))


Nit: I know that you are following the existing pattern, but these IsImageVersionAtLeast checks are unnecessary. (I think I have made the same comment the last time new structure got added here.)

jkotas · 2019-05-17T02:38:18Z

Build breaks:

/__w/1/s/src/zap/../vm/nativeformatreader.h:24:10: fatal error: 'arm64_neon.h' file not found
#include "arm64_neon.h"

sergiy-k · 2019-05-20T17:01:26Z

What else needs to be done here before this PR can be merged?

janvorli

LGTM, I only found a small nit that you may ignore.

janvorli · 2019-05-20T21:31:22Z

        if (!IsWinMD()) // ignore classic COM interop CAs in .winmd
        {
-            if (this->GetManifestImport()->GetCustomAttributeByName(TokenFromRid(1, mdtAssembly), INTEROP_IMPORTEDFROMTYPELIB_TYPE, 0, 0) == S_OK)
+            if (GetManifestModule()->GetCustomAttribute(TokenFromRid(1, mdtAssembly), WellKnownAttribute::ImportedFromTypeLib, NULL, 0) == S_OK)


A nit - maybe we can use the Assembly::GetCustomAttribute wrapper here to make the line little less verbose. The same for the occurrence in the assembly.cpp above.

* Basic infra for cuckoo filter of attributes - Implement cuckoo filter lookup logic - Implement new ready to run section - Add dumper to R2RDump - Parse section on load into data structure - Implement function to query filter - Add concept of enum of well known attributes - So that attribute name hashes themselves may be cached * Wrap all even vaguely perf critical uses of attribute by name parsing with use of R2R data * Update emmintrin.h in the PAL header to contain the needed SSE2 intrinsics for the feature - Disable the presence table for non Corelib cases. Current performance data does not warrant the size increase in other generated binaries Commit migrated from dotnet/coreclr@adecd85

davidwrighton added 6 commits May 5, 2019 19:15

Wrap all even vaguely perf critical uses of attribute by name parsing…

37378d7

… with use of R2R data

Fixup well known attributes, sort by namespace qualified name

7c7339d

All reading of cuckoo filter scenarios build

5a4eb69

Silence compiler warning about CountOfWellKnownAttributes

f370c5c

This should fix the current error seen when compiling on Linux. The OSX issue with immintrin.h will be dealt with later.

According the Intel Intrinsics guide immintrin shouldn't be necessary

5a9b0a5

tannergooding reviewed May 9, 2019

View reviewed changes

Update emmintrin.h header to contain the needed SSE2 intrinsics for t…

2eede30

…he feature

davidwrighton added 5 commits May 13, 2019 07:51

Actually generate custom attribute filter

dcd3ebb

Merge branch 'cuckoo_metadata' of https://github.com/davidwrighton/co…

9471d67

…reclr into cuckoo_metadata

Force filters to be power of two in size

86e0a57

- With current xor based second hash system, cuckoo filters must be powers of two. This can be addressed later, after performance is analyzed.

Add support for ARM NEON intrinsic processing of the cuckoo filter

6e787ec

- Probably doesn't work on *nix platforms, but I'll discover that in the cloud

Use arm64_neon.h header instead of arm_neon header on ARM64

cc7b703

janvorli reviewed May 15, 2019

View reviewed changes

davidwrighton added 5 commits May 15, 2019 11:51

Disable SIMD for ARM and ARM64 on Linux and Mac

72c3d1f

- Our build environments aren't setup for NEON intrinsics - Performance win is affected by using non-SIMD path, but the cuckoo filter is still an overall win. Once this is all checked in, I'll open an issue to enable SIMD in our build.

Make attributes filter resilient to metadata modification by the prof…

1d7b292

…iler

Fix typos and copypasta

f520e47

Cuckoo filter is power of two in size always

86110db

Results due to performance measurement

697c2b3

- Fix the disable filter logic - Disable the presence table for non Corelib cases. Current performance data does not warrant the size increase in other generated binaries

davidwrighton changed the title ~~[WIP]Cuckoo metadata~~ Cuckoo metadata May 17, 2019

davidwrighton marked this pull request as ready for review May 17, 2019 01:41

davidwrighton added 2 commits May 16, 2019 18:48

Merge branch 'master' of https://github.com/dotnet/coreclr into cucko…

bd369b2

…o_metadata Attempted to address conflict with native type parsing

Fix merge issues

211037f

jkotas reviewed May 17, 2019

View reviewed changes

Address feedback

e495713

janvorli approved these changes May 20, 2019

View reviewed changes

davidwrighton merged commit adecd85 into dotnet:master May 20, 2019

kotlarmilos mentioned this pull request Mar 27, 2023

[mono] Implement cuckoo filter for attribute retrieval dotnet/runtime#83974

Closed


		#ifndef DACCESS_COMPILE

		#if defined(_M_X64) \|\| defined(__x86_64__) \|\| defined(__i386__) \|\| defined(_M_IX86)


		/*

		The below hash combining function is based on the xxHash32 logic implemented

Uh oh!

Conversation

davidwrighton commented May 9, 2019

Uh oh!

tannergooding commented May 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton commented May 10, 2019

Uh oh!

tannergooding commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented May 10, 2019

Uh oh!

davidwrighton commented May 11, 2019

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!

davidwrighton commented May 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented May 17, 2019

Uh oh!

sergiy-k commented May 20, 2019

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tannergooding commented May 10, 2019 •

edited

Loading