Skip to content

Add unit tests for hashtable with classes as keys #123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Conversation

josesimoes
Copy link
Member

Description

  • Add unit tests for hashtable with classes as keys.

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

@TerryFogg
Copy link

Maybe the test doesn't produce the type as a class which it fails on with the WPF example

@alberk8
Copy link

alberk8 commented Dec 1, 2023

I tried the code on Real Hardware and it failed with a system crash. It crashed on the eight iteration of adding into HashTable.

Debug Trace

0x401c0898: HAL_AssertEx at /workspaces/nf-interpreter/targets/ESP32/_common/targetHAL.c:11
0x400d3448: CLR_RT_HeapBlock::ObjectsEqual(CLR_RT_HeapBlock const&, CLR_RT_HeapBlock const&, bool) at /workspaces/nf-interpreter/src/CLR/Core/CLR_RT_HeapBlock.cpp:1385
0x40102f7d: Library_nf_system_collections_System_Collections_Hashtable::InsertNative___VOID__OBJECT__OBJECT__BOOLEAN(CLR_RT_StackFrame&) at /workspaces/nf-interpreter/src/nanoFramework.System.Collections/nf_system_collections_System_Collections_Hashtable.cpp:261
0x400e224e: CLR_RT_Thread::Execute_Inner() at /workspaces/nf-interpreter/src/CLR/Core/Interpreter.cpp:843
0x400e1fa5: CLR_RT_Thread::Execute() at /workspaces/nf-interpreter/src/CLR/Core/Interpreter.cpp:645
0x400dcd61: CLR_RT_ExecutionEngine::ScheduleThreads(int) at /workspaces/nf-interpreter/src/CLR/Core/Execution.cpp:1246
0x400dc4e9: CLR_RT_ExecutionEngine::Execute(wchar_t*, int) at /workspaces/nf-interpreter/src/CLR/Core/Execution.cpp:673
0x400f5872: ClrStartup at /workspaces/nf-interpreter/src/CLR/Startup/CLRStartup.cpp:407
0x4011b021: CLRStartupThread at /workspaces/nf-interpreter/targets/ESP32/_nanoCLR/CLR_Startup_Thread.c:17
0x401190d2: main_task at /workspaces/nf-interpreter/targets/ESP32/_IDF/esp32/app_main.c:37

@josesimoes josesimoes force-pushed the add-unit-test branch 2 times, most recently from a439c79 to fc892eb Compare December 1, 2023 09:23
Copy link

sonarqubecloud bot commented Dec 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Ellerbach
Copy link
Member

Note

Investigation run on an ESP32 with PSRAM with ESP32_PSRAM_REV0-1.9.0.118 firmware.

I've been playing a bit with all this and here is the finding:

  • The elements fails to be added because of a capacity increase issue
  • If you increase manually the initial capacity, then it fails later.

Here is the simple code:

using System;
using System.Collections;

AnotherTest();

void AnotherTest()
{
    // This is what we will adjust
    var hashtable = new Hashtable(1000);

    Console.WriteLine("Adding 100 elements to the hashtable with SomeOtherKey class");

    for (int i = 0; i < 100; i++)
    {
        SomeOtherKey keyt = new SomeOtherKey(i);

        hashtable.Add(keyt, keyt.I.GetHashCode());
    }

    foreach (var keyo in hashtable.Keys)
    {
        if ((keyo as SomeOtherKey).I.GetHashCode() != (int)hashtable[keyo])
        {
            Console.WriteLine($"Failed on the {keyo} key.");
        }
    }
}

This fails at 25th element!

If you remove the 1000 from the initialisation of the Hashtable, this now fails at 4!

With initialization of 5000, 67th

OK, now you got the math so var hashtable = new Hashtable(7500); will perfectly work.

But what if you have 150 elements? Well, it will then always fails because var hashtable = new Hashtable(9000); will fail.
So, there is definitely an issue in scaling up the capacity automatically.

@alberk8
Copy link

alberk8 commented Dec 2, 2023

Can I confirm that if the Class has Constructor then you cannot directly get the GetHashCode and add into the HashTable?

Code Works

SomeKey key = new SomeKey();
hashtable.Add(key, key.GetHashCode());

Code Fails

  SomeOtherKey key = new SomeOtherKey(i);
  hashtable.Add(key, key.GetHashCode());

@josesimoes
Copy link
Member Author

@alberk8 did you run this before or after the fix was merged? I just run it from an STM32F769 DISCO and ESP_WROVER_KIT fw 1.9.0.118 without any issues... key with or without constructor, getting hash from it, etc

@alberk8
Copy link

alberk8 commented Dec 4, 2023

@josesimoes , I tried again on ESP32_REV3 v1.9.0.901 and it crash on the the third iteration.

Error

Loop 0
Loop 1
Loop 2
    ++++ Exception System.ArgumentException - 0xfd000000 (1) ++++
    ++++ Message: 
    ++++ System.Collections.Hashtable::InsertNative [IP: 0000] ++++
    ++++ System.Collections.Hashtable::Add [IP: 0007] ++++
    ++++ nanoFramework_HashTableTest.Test::Hashtable_ClassAsKey [IP: 0030] ++++
    ++++ nanoFramework_HashTableTest.Program::Main [IP: 0014] ++++
Exception thrown: 'System.ArgumentException' in nanoFramework.System.Collections.dll
An unhandled exception of type 'System.ArgumentException' occurred in nanoFramework.System.Collections.dll

The program '[1] .NET nanoFramework application: Managed' has exited with code 0 (0x0).

Code

public static class Test
{
    public static void Hashtable_ClassAsKey()
    {
        var hashtable = new Hashtable();

        for (int i = 0; i < 10; i++)
        {
            Debug.WriteLine($"Loop {i}");
            SomeKey key = new SomeKey(i);
            hashtable.Add(key, key.GetHashCode());
          }

        foreach (var key in hashtable.Keys)
        {
            Debug.WriteLine($"{key.GetHashCode()} -> {(int)hashtable[key]}");
         }
    }
}

public class SomeKey1 { }

public class SomeKey
{
    private readonly int _value;
    public SomeKey() { }

    public SomeKey(int value) 
    {
        _value = value; 
    }
    public int GetValue => _value;
}

@josesimoes
Copy link
Member Author

Interesting fact following several tests on multiple platforms:

  • ESP32 failing when adding the 64 element
  • STM32 failing when adding the 74 element
  • virtual device failin when adding the 78 element

🤔

@alberk8
Copy link

alberk8 commented Dec 5, 2023

@josesimoes , I added Debug.WriteLine("Hash Code " + key.GetHashCode()); before the hashtable.Add() and it can run without issue upto 1000 iterations, More mystery.

Could it be the HashCode is not ready when it is trying to add into the hashtable.

@josesimoes
Copy link
Member Author

@alberk8 root cause found and fixed. Thank you for the prompt reply as usual. 👍🏻

@josesimoes
Copy link
Member Author

This should be fixed now with nanoframework/nf-interpreter#2838

@Ellerbach
Copy link
Member

I can happily report that with ESP32_PSRAM_REV0-1.9.0.122.zip the sample code is working.

But I found another issue.Adjust the SomeOtherKey class like this:

public class SomeOtherKey
{
    // with or without the private i, the behavior is the same
    //private readonly int _i;

    public int I { get; private set; }//=> _i;
    public int J { get; private set; }//=> _i;

    //public SomeOtherKey()
    //{
    //}

    public SomeOtherKey(int i)
    {
        //_i = i;
        I = i;
    }
}

and then I get a "Resolving" and it does not even do the first Add properly.

The nanoDevice runtime is loading the application assemblies and starting execution.
'nanoFramework.Tools.VS2022.Extension.dll' (Managed): Loaded 'C:\tmp\TestCollection\TestCollection\packages\nanoFramework.CoreLibrary.1.15.5\lib\mscorlib.dll', Skipped loading symbols. Module is optimized and the debugger option 'Just My Code' is enabled.
'nanoFramework.Tools.VS2022.Extension.dll' (Managed): Loaded 'C:\tmp\TestCollection\TestCollection\TestCollection\bin\Debug\TestCollection.exe', Symbols loaded.
'nanoFramework.Tools.VS2022.Extension.dll' (Managed): Loaded 'C:\tmp\TestCollection\TestCollection\packages\nanoFramework.Runtime.Native.1.6.12\lib\nanoFramework.Runtime.Native.dll', Skipped loading symbols. Module is optimized and the debugger option 'Just My Code' is enabled.
'nanoFramework.Tools.VS2022.Extension.dll' (Managed): Loaded 'C:\tmp\TestCollection\TestCollection\packages\nanoFramework.System.Collections.1.5.31\lib\nanoFramework.System.Collections.dll', Skipped loading symbols. Module is optimized and the debugger option 'Just My Code' is enabled.
The thread '<No Name>' (0x2) has exited with code 0 (0x0).
Adding 100 elements to the hashtable with SomeOtherKey class
Resolving.

Total: (4452 RAM - 39284 ROM - 22921 METADATA)

@alberk8
Copy link

alberk8 commented Dec 6, 2023

@josesimoes , Testing with the latest ESP32_REV3 v1.9.0.908 firmware with the test code above runs but the hashtable key-value does not match.

1507231066 -> 635863792
1540282740 -> -1800014665
1908457812 -> 248935635
-80404239 -> -1124769067
622439050 -> 471702695
-1718675944 -> -2127738934
848934942 -> -383884899
-1770463702 -> 724790273
1617832763 -> 1629645558
1482383836 -> -815670530
1578399071 -> 1295485284

Update
Inserting a Debug.WriteLine("HashCode: " + key.GetHashCode()); before the hashtable.Add corrected the hashcode matching.
Replacing the Debug.WriteLine with var x = key.GetHashCode(); does not work but var x = key.GetHashCode().ToString(); works.

@josesimoes
Copy link
Member Author

josesimoes commented Dec 6, 2023

@alberk8 confirmed. This should be an issue with the hashcode generation, something failing for classes.

With this slight variation:

        public static void Hashtable_ClassAsKey()
        {
            var hashtable = new Hashtable();

            for (int i = 0; i < 10; i++)
            {
                SomeKey key = new SomeKey(i);

                hashtable.Add(key, i.GetHashCode());

                Debug.WriteLine($"Added {i} -> {i.GetHashCode()} element.");
            }

            Debug.WriteLine($"Added 10 elements.");

            foreach (SomeKey key in hashtable.Keys)
            {
                Debug.WriteLine($"{(key as SomeKey).GetValue.GetHashCode()} -> {(int)hashtable[key]} -> {(key as SomeKey).GetValue}");
            }
        }

I get this:

Hello from nanoFramework!
Added 0 -> 0 element.
Added 1 -> 1 element.
Added 2 -> 2 element.
Added 3 -> 3 element.
Added 4 -> 4 element.
Added 5 -> 5 element.
Added 6 -> 6 element.
Added 7 -> 7 element.
Added 8 -> 8 element.
Added 9 -> 9 element.
Added 10 elements.
7 -> 7 -> 7
1 -> 1 -> 1
6 -> 6 -> 6
3 -> 3 -> 3
2 -> 2 -> 2
0 -> 0 -> 0
4 -> 4 -> 4
8 -> 8 -> 8
5 -> 5 -> 5
9 -> 9 -> 9

When using a class:

Hello from nanoFramework!
Added 0 -> 888889346 element.
Added 1 -> 517421212 element.
Added 2 -> 1905006634 element.
Added 3 -> 1891387079 element.
Added 4 -> -1347221031 element.
Added 5 -> -495693417 element.
Added 6 -> -1012995772 element.
Added 7 -> -203118857 element.
Added 8 -> -586458527 element.
Added 9 -> -708915919 element.
Added 10 elements.
517421212 -> 2008192259 -> 1
1905006634 -> 849409522 -> 2
-586458527 -> 1002131715 -> 8
888889346 -> -1074071709 -> 0
1891387079 -> 1784495446 -> 3
-708915919 -> -620416548 -> 9
-203118857 -> -1077162069 -> 7
-1347221031 -> -567337183 -> 4
-1012995772 -> -2015027439 -> 6
-495693417 -> -660831182 -> 5

@alberk8
Copy link

alberk8 commented Dec 7, 2023

@josesimoes Testing the latest firmware ESP32_REV3 v1.9.0.910 has another issue. The class below GetHashCode returns Zero.

public class SomeKey
{
public int GetValue { get; set; }
}

@josesimoes
Copy link
Member Author

@alberk8 say again please....

@alberk8
Copy link

alberk8 commented Dec 7, 2023

@josesimoes , Sorry the above class works. But If I have more than one property then the HashCode for the class is Zero.

Debug Output

HashKey Count: 10
0 -> 0
0 -> 0
0 -> 0
0 -> 0
0 -> 0
0 -> 0
0 -> 0
0 -> 0
0 -> 0
0 -> 0
XXXXX Done 

Code

 public static class Test
 {
     public static void Hashtable_ClassAsKey()
     {
         var hashtable = new Hashtable();

         for (int i = 0; i < 10; i++)
         {
             SomeKey key = new SomeKey();
             hashtable.Add(key, key.GetHashCode());
         }

         Debug.WriteLine("HashKey Count: " + hashtable.Count);   
         foreach (var key in hashtable.Keys)
         {
             Debug.WriteLine($"{key.GetHashCode()} -> {(int)hashtable[key]}");
         }

         Console.WriteLine("XXXXX Done ");
     }
 }

 public class SomeKey
 {
     public string Name { get; set; }
     public int GetValue { get; set; }
 }

@josesimoes
Copy link
Member Author

Having no more feedback about this and considering the Unit Tests are now 🟢 after the fixes in native code, I'm merging this.

@josesimoes josesimoes merged commit c0ec265 into main Dec 13, 2023
@josesimoes josesimoes deleted the add-unit-test branch December 13, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants