[PATCH v2 2/4] vbscript: Move the script context heap to the script dispatch object.

[Gabriel Ivăncescu]

On 10/24/19 11:22 PM, Jacek Caban wrote:
> On 10/24/19 3:32 PM, Gabriel Ivăncescu wrote:
>> Hi Jacek,
>>
>> On 10/23/19 9:16 PM, Jacek Caban wrote:
>>> Hi Gabriel,
>>>
>>> On 10/21/19 1:40 PM, Gabriel Ivăncescu wrote:
>>>> The heap stores dynamic global variables added during the execution
>>>> phase. We'll need to reference them in the TypeInfo, and so we have to
>>>> reference count the heap itself, and the simplest way is to use the 
>>>> script
>>>> dispatch object to store it.
>>>
>>>
>>> It seems to me that you concentrated a bit too much at avoiding 
>>> copying the data rather than on root of the problem. It's possible to 
>>> avoid copying in different ways. The thing is that having to 
>>> represent the collection of global members in different ways for 
>>> similar tasks probably means that representation of the collection is 
>>> not right. Actually, we know that current global scope representation 
>>> is not great. A simple list was nice to have things done and working, 
>>> but it's surely not optimal. I don't expect you to rewrite it all, 
>>> but it will be eventually rewritten and it would be good to know that 
>>> we're changing things in the right direction instead of working on 
>>> something that will need to be rewritten for future improvements.
>>>
>>
>> Now that I've looked deeper into the vbscript implementation, I 
>> actually think the current one is wrong and don't quite understand how 
>> it even works, honestly.
>>
>> It looks to me as the compilation phase that creates the vbscode_t 
>> that is then appened to the ctx->code_list adds functions and 
>> variables, and they are immediately linked to the context's 
>> global_funcs/vars respectively. This happens even *if* the code in 
>> question has "pending_exec" so it's not even executed immediately. 
>> Shouldn't those funcs/vars only be added to the global context list 
>> when they are being executed?
> 
> 
> Maybe, I'm not sure. It's not obviously wrong. Testing that would be nice.
> 
> 
>>
>> Furthermore, the funcs/vars during compilation are added to the 
>> vbscode_t's heap and stored there, while vars added during execution 
>> are added to the context's heap.
>>
>> release_script will go through all global_vars to release their 
>> variant data, but in destroy_script, the code_list is released (and 
>> all vbscode_t heaps) *before* the script is released. Doesn't this 
>> mean some variables in the linked list will have already been freed 
>> (if they were part of some vbscode_t) and thus we're accessing freed 
>> memory when releasing the vars?
>>
>> Overall I'm just slightly confused as to how it even works right now, 
>> so I've no idea how to improve it.
> 
> 
> Well, it definitely could use a clean up. Note that before 
> destroy_script() we always set the state to closed in the destructor, so 
> in practice the release_script() is always called before 
> destroy_script() and there is no problem. We should eventually get rid 
> of destroy_script(). If we want script_ctx_t ref-counted, it could be a 
> part of release_script() and we could have separated detach_script() 
> that would be used for closing script engine.
> 
> 

Right, so then it means it isn't broken currently, just that the 
release_script in destroy_script is confusing and probably redundant.

>>
>> Here's my thought process on how to proceed, but I could be completely 
>> wrong.
>>
>> First, store the global_vars/funcs in the script dispatch object, 
>> instead of the context (just like jscript does). Furthermore, when a 
>> code in the code_list is being executed, move it to the script 
>> dispatch object (for ref counting) and remove it from the context's 
>> list. (we'll essentially just have another list in the script dispatch 
>> object for code that has been executed).
>>
>> This will allow to release the script by decreasing its state, 
>> releasing the code that was executed already, while *keeping* the code 
>> that is still pending, which I believe was the intent of the whole 
>> thing. Again, I could be misunderstanding this.
> 
> 
> I'm not sure. We don't really need the whole vbscode_t in script 
> dispatch. For the variables, we just need just a name. For function, 
> it's a name, public flag and argument count. The rest could be left in 
> vbscode_t and be freed with it on script engine close. We could allocate 
> that subset of data, for example, in a dedicated heap pool like we 
> currently do for dynamic variables. We could do that in the compiler 
> already.
> 
> 

I think keeping all of the vbscode_t is simpler and shouldn't really 
matter in general, since they should theoretically also be part of the 
script dispatch (but current code can't handle it when engine is 
closed), for GetIDsOfNames and so on.

Note that we also need to keep the argument names, not just the count.

To make this more straightforward, I was further thinking of getting rid 
of the "pending_exec" field.

Then, we'll have a pending_code_list (not sure if in context or 
dispatch, I'm leaning towards dispatch, see next paragraph), and a 
code_list in the script dispatch. Code that was executed simply gets 
moved to the code_list in the script dispatch (since it's part of it) 
from the pending_code_list.

In the future, this will probably help as well if we make use of the 
pstrItemName arg in GetScriptDispatch, since it should separate 
functions/vars added to it (and thus, they need to be part of the 
dispatch object itself rather than the context). I noticed this when I 
realized the "modules" in msscript makes use of this argument to 
separate functions and vars.

But of course I'll need to do more testing with this, I do believe it's 
more correct, at least based on intuition.

> 
> To get rid of ident_map, I'd suggest to change how DISPIDs work. DISPIDs 
> could be just indexes in variable or function arrays, with a flag on one 
> of high bits indicating which one is it.
> 

Seems reasonable. I suppose we could still keep linked lists in 
vbscode_t since they'll have to be appended to the arrays after 
compilation (or during execution, if tests confirm it).

Thanks,
Gabriel