VBA-tools / VBA-JSON

JSON conversion and parsing for VBA
MIT License
1.76k stars 568 forks source link

Late binding #219

Open Deedolith opened 2 years ago

Deedolith commented 2 years ago

Suggestion to switch the library to Late binding to prevent known issues with Early bound libraries (type names conflicts).

Nick-vanGemeren commented 2 years ago

Surely this should be a compilation option? ParseJSONcan create manyDictionary.

stenci commented 2 years ago

I don't agree with this PR, it has a horrible effect on performance. Late binding can be hundreds of times slower than early binding.

The solution to the naming conflict is not to introduce sloppy performance problems, it is to not use ambiguous names, that is use the library name prefix.

Here is a quick test that shows how bad late binding can be:

Sub Test()
  Dim T As Single, O As Object, I As Long, D As Dictionary

  T = Timer
  For I = 1 To 10000
    Set O = CreateObject("Scripting.Dictionary")
    O.Exists "key"
  Next I
  Debug.Print Format(Timer - T, "0.000")

  T = Timer
  For I = 1 To 10000
    Set D = New Dictionary
    D.Exists "key"
  Next I
  Debug.Print Format(Timer - T, "0.000")
End Sub

Output:

3.453
0.039
Deedolith commented 2 years ago

Afraid this is a naive approach. First, performances issues must be analysed with profiling tools, and they must be the least of your concerns during development steps (readability and producing the expected behavior are the top ones).

Second, name conflict are not always solved as easilly as you think (immagine the nightmare on project with million of lines of codes and more dependencies than fingers on your hands and feets), impacts can go from minimal (easy case) to hundred of days of work (nightmare case).

Finally, name conflict is only one of the issue provided by early binding, version conflict is probably the worst one (remember that developers have no controls on end user computers).

stenci commented 2 years ago

I have been using this module for years. In the past only for small data structures because it was unusable on some computers because it was too slow. Converting the same dictionary to JSON on my computer and a few other ones would take 1 second, but there were 5-6 computers on my company where it would take 15 minutes.

After upgrading to one of the latest revisions (I don't remember which one, I think there were improvements on how the buffers are managed) all of a sudden it became fast on all computers and I have finally started using it also with large data structures.

Switching from early to late binding would force me to abandon this module again and go back to my personal tricks to solve performance problems.

The test in my previous post has only 10000 calls to one method. I often have data structures that require much more method calls, late binding would make this module unusably slow and I would need to go back to my old contraption (or keep my branch with early binding :) )


True, name conflicts are not always solved so easily, but declaring the namespace is most of the time the first thing to try and the most likely to succeed (at least in my experience).


True, readability is very important, and both As Dictionary and New Dictionary are definitely more readable than As Object and CreateObject("Scripting.Dictionary").


True, producing the expected behavior is very important, and I can assure you that hiding behind late binding is the last resource and should only be used to when different computers could have different and incompatible versions of the same component, and the add-in uses only methods that are compatible across all the versions. But a Dictionary from scrrun.dll is definitely reliable enough to be available and compatible across any respectable computer.

Nick-vanGemeren commented 2 years ago

I think that, as far as possible, any changes to the existing behaviour of modules should be driven by options, the existing behaviour remaining the default. In this case, to avoid a compiler error, we need to use conditional compilation.

I hope we can agree thatjson_ParseObjectcan return an Object rather than Dictionary.

I propose:

‘ Compilation option:
‘----
‘ DictionaryBinding: can specify early or late binding of Scripting.Dictionary.
 ‘  MS Word: import VBA-Dictionary or define non-zero DictionaryBinding
‘   Mac: import VBA-Dictionary (Scripting.Dictionary not available)
 ‘  Default (0): use Dictionary class (VBA-Dictionary or early-bound Scripting.Dictionary)
‘   To set, uncomment one of the following or define in the VBA project Properties (colon-separated list)
‘#Const DictionaryBinding = 1     ‘ Early bind Scripting.Dictionary 
‘#Const DictionaryBinding = -1    ‘ Late bind Scripting.Dictionary
‘----

and then injson_ParseObject:

#If Mac Then
    Set json_ParseObject = New Dictionary
#ElseIf DictionaryBinding = 1 Then
    Set json_ParseObject = New Scripting.Dictionary
#ElseIf DictionaryBinding = -1 Then
    Set json_ParseObject = CreateObject("Scripting.Dictionary")
#Else
    Set json_ParseObject = New Dictionary
#End If

This is backwards compatible and allows full flexibility for Windows users. No code modification is needed if the project properties are used.