Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/coreclr/vm/wasm/callhelpers-interp-to-managed.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,6 @@ const StringToWasmSigThunk g_wasmThunks[] = {
{ "viiinni", (void*)&CallFunc_I32_I32_I32_IND_IND_I32_RetVoid },
{ "viin", (void*)&CallFunc_I32_I32_IND_RetVoid },
{ "viinni", (void*)&CallFunc_I32_I32_IND_IND_I32_RetVoid },
{ "viin", (void*)&CallFunc_I32_I32_IND_RetVoid },
{ "viinnii", (void*)&CallFunc_I32_I32_IND_IND_I32_I32_RetVoid },
{ "vin", (void*)&CallFunc_I32_IND_RetVoid },
{ "vini", (void*)&CallFunc_I32_IND_I32_RetVoid },
Expand Down
221 changes: 208 additions & 13 deletions src/coreclr/vm/wasm/callhelpers-reverse.cpp

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/coreclr/vm/wasm/callhelpers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ struct ReverseThunkMapValue
struct ReverseThunkMapEntry
{
ULONG key;
ULONG fallbackKey;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, having trimming enabled makes the primary key irrelevant. I'm assuming that having trimming enabled is a must have default on wasm. So does it even make sense to have 2 keys in the first place ?

ReverseThunkMapValue value;
};

Expand Down
77 changes: 62 additions & 15 deletions src/coreclr/vm/wasm/helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -623,10 +623,33 @@ namespace
if (!GetSignatureKey(sig, keyBuffer, keyBufferLen))
return NULL;

return LookupThunk(keyBuffer);
void* thunk = LookupThunk(keyBuffer);
#ifdef _DEBUG
if (thunk == NULL)
printf("WASM calli missing for key: %s\n", keyBuffer);
#endif
return thunk;
}

ULONG CreateFallbackKey(MethodDesc* pMD)
{
_ASSERTE(pMD != nullptr);

// the fallback key is in the form $"{MethodName}#{Method.GetParameters().Length}:{AssemblyName}:{Namespace}:{TypeName}";
LPCUTF8 pszNamespace = nullptr;
LPCUTF8 pszName = pMD->GetMethodTable()->GetFullyQualifiedNameInfo(&pszNamespace);
MetaSig sig(pMD);
SString strFullName;
strFullName.Printf("%s#%d:%s:%s:%s",
pMD->GetName(),
sig.NumFixedArgs(),
pMD->GetAssembly()->GetSimpleName(),
pszNamespace != nullptr ? pszNamespace : "",
pszName);

return strFullName.Hash();
}

// TODO: This hashing function should be replaced.
ULONG CreateKey(MethodDesc* pMD)
{
_ASSERTE(pMD != nullptr);
Expand All @@ -645,30 +668,54 @@ namespace

typedef MapSHash<ULONG, const ReverseThunkMapValue*> HashToReverseThunkHash;
HashToReverseThunkHash* reverseThunkCache = nullptr;
HashToReverseThunkHash* reverseThunkFallbackCache = nullptr;

HashToReverseThunkHash* CreateReverseThunkHashTable(bool fallback)
{
HashToReverseThunkHash* newTable = new HashToReverseThunkHash();
newTable->Reallocate(g_ReverseThunksCount * HashToReverseThunkHash::s_density_factor_denominator / HashToReverseThunkHash::s_density_factor_numerator + 1);
for (size_t i = 0; i < g_ReverseThunksCount; i++)
{
newTable->Add(fallback ? g_ReverseThunks[i].fallbackKey : g_ReverseThunks[i].key, &g_ReverseThunks[i].value);
}

HashToReverseThunkHash **ppCache = fallback ? &reverseThunkFallbackCache : &reverseThunkCache;
if (InterlockedCompareExchangeT(ppCache, newTable, nullptr) != nullptr)
{
// Another thread won the race, discard ours
delete newTable;
}
return *ppCache;
}

const ReverseThunkMapValue* LookupThunk(MethodDesc* pMD)
{
HashToReverseThunkHash* table = VolatileLoad(&reverseThunkCache);
if (table == nullptr)
{
HashToReverseThunkHash* newTable = new HashToReverseThunkHash();
newTable->Reallocate(g_ReverseThunksCount * HashToReverseThunkHash::s_density_factor_denominator / HashToReverseThunkHash::s_density_factor_numerator + 1);
for (size_t i = 0; i < g_ReverseThunksCount; i++)
{
newTable->Add(g_ReverseThunks[i].key, &g_ReverseThunks[i].value);
}

if (InterlockedCompareExchangeT(&reverseThunkCache, newTable, nullptr) != nullptr)
{
// Another thread won the race, discard ours
delete newTable;
}
table = reverseThunkCache;
table = CreateReverseThunkHashTable(false /* fallback */);
}

ULONG key = CreateKey(pMD);

// Try primary key, it is based on Assembly fully qualified name and method token
const ReverseThunkMapValue* thunk;
if (table->Lookup(key, &thunk))
{
return thunk;
}

// Try fallback key, that is based on method properties and assembly name
// The fallback is used when the assembly is trimmed and the token and assembly fully qualified name
// may change.
table = VolatileLoad(&reverseThunkFallbackCache);
if (table == nullptr)
{
table = CreateReverseThunkHashTable(true /* fallback */);
}

key = CreateFallbackKey(pMD);

Comment on lines +701 to +718
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we get a key collision? Is it mechanically prevented from happening somewhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking deeper, I'm concerned that we're using a low quality 32-bit hash here, so I would hope that at least if we get a hash collision during thunk lookup, it will fail deterministically with a clear error message so we can go back and fix it. Ideally we would use a more robust 64-bit hash to make the odds of a collision way lower, or do an actual string check. But I don't know if thunk lookup is on a hot path to the point that we can't afford it.

bool success = table->Lookup(key, &thunk);
return success ? thunk : nullptr;
}
Expand Down
18 changes: 6 additions & 12 deletions src/tasks/WasmAppBuilder/WasmAppBuilder.csproj
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppToolCurrent);$(NetFrameworkToolCurrent)</TargetFrameworks>
<TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
<Nullable>enable</Nullable>
<NoWarn>$(NoWarn),CA1050</NoWarn>
<!-- Ignore nullable warnings on net4* -->
<NoWarn Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">$(NoWarn),CS8604,CS8602</NoWarn>
<NoWarn Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) != '.NETFramework'">$(NoWarn),CA1850</NoWarn>
<NoWarn>$(NoWarn),CA1850</NoWarn>
<EnableSingleFileAnalyzer>false</EnableSingleFileAnalyzer>
<CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
Expand Down Expand Up @@ -46,19 +45,14 @@
<Target Name="GetFilesToPackage" Returns="@(FilesToPackage)">
<ItemGroup>
<!-- .NETCoreApp -->
<FilesToPackage Include="$(OutputPath)$(NetCoreAppToolCurrent)\$(MSBuildProjectName)*"
<FilesToPackage Include="$(OutputPath)$(MSBuildProjectName)*"
TargetPath="tasks\$(NetCoreAppToolCurrent)" />
<FilesToPackage Include="$(OutputPath)$(NetCoreAppToolCurrent)\Microsoft.NET.WebAssembly.Webcil.dll"
<FilesToPackage Include="$(OutputPath)Microsoft.NET.WebAssembly.Webcil.dll"
TargetPath="tasks\$(NetCoreAppToolCurrent)" />


<!-- For .NETFramework we need all the dependent assemblies too, so copy from the publish folder -->
<FilesToPackage Include="$(OutputPath)$(NetFrameworkToolCurrent)\*"
TargetPath="tasks\$(NetFrameworkToolCurrent)" />
</ItemGroup>
</Target>

<UsingTask TaskName="ManagedToNativeGenerator" Runtime="NET" TaskFactory="TaskHostFactory" AssemblyFile="$(OutputPath)$(NetCoreAppToolCurrent)\WasmAppBuilder.dll" />
<UsingTask TaskName="ManagedToNativeGenerator" Runtime="NET" TaskFactory="TaskHostFactory" AssemblyFile="$(OutputPath)\WasmAppBuilder.dll" />

<Target Name="RunGenerator" DependsOnTargets="Build" Condition="'$(AssembliesScanPath)' != ''">
<ItemGroup>
Expand All @@ -73,7 +67,7 @@
<ManagedToNativeGenerator
Assemblies="@(WasmPInvokeAssembly)"
PInvokeModules="@(WasmPInvokeModule)"
PInvokeOutputPath="$(GeneratorOutputPath)todo-pinvoke-helpers.cpp"
PInvokeOutputPath="$(GeneratorOutputPath)callhelpers-reverse.cpp"
InterpToNativeOutputPath="$(GeneratorOutputPath)callhelpers-interp-to-managed.cpp">
<Output TaskParameter="FileWrites" ItemName="FileWrites" />
</ManagedToNativeGenerator>
Expand Down
Loading