backedge checks

[Edward Nevill]

Hi,

Apologies for the legal notices that appeared in my previous emails. I am now doing this
from my home machine over a wet piece of string. Email address is now either ed at camswl.com
or ed at starksfield.org (both resolve to the same machine).

>>           INCR_INVOCATION_COUNT;\
>>           SAFEPOINT;\
>>         }\
>>       }  /* UseCompiler ... */\
>>       INCR_INVOCATION_COUNT;\
>>       SAFEPOINT;\
>>     }
>> 
>> 
>> This macro is invoked in every branch (although the body is only
>> executed on backwards branches).
>> 
>> Firstly, is it just me, or does it do INCR_INVOCATION_COUNT; SAFEPOINT
>> twice on every backwards branch. Surely a mistake?
>
>It looks like that to me.

OK. Can someone with committal rights remove the first instance.

>> Secondly, can someone tell me under what circumstances in zero
>> UseCompiler would ever be true (and no, it doesn't resolve to a
>> constant.
>
>This stuff is AFAIK used to decide when to compile a method with Shark.

OK. So Shark is using the bytecode interpreter rather than the template interpreter?
Is this correct Gary?

In that case it is even more important we do something about the dire performance.

Regardless, when we are building zero (interpreted) this should be defined to be constant
false. At the moment it is a global.

Aside:

In general access to globals is very expensive (more expensive than you might believe in a
PIC world). If we need to do checks on globals can we at least do..

{
	type	*UseCompiler_ptr = &UseCompiler;

	...
	if (*UseCompiler_ptr) {
		...
	}
}

This is partly doing the compilers work for it, but sometimes it needs help.

>> notice_safepoints()
>> {
>>                 update_table(safe_branch_dispatch_table,
>> 			safe_branch_dispatch_table +
>>
>sizeof(safe_branch_dispatch_table)/sizeof(branch_dispatch));
>> }
>> 
>> ignore_safepoint()
>> {
>>                 update_table(unsafe_branch_dispatch_table,
>> 		unsafe_branch_dispatch_table +
>>
>sizeof(unsafe_branch_dispatch_table)/sizeof(branch_dispatch));
>> }
>
>Hmm, this doesn't look like it's thread safe to me.  It would make more
>sense to have a pointer to the branch dispatch table that's updated
>atomically.

There is a problem with updating the pointer to the table rather than the contents
of the table.

Hopefully, the pointer to the table is residing in an ARM register (or MIPS or ...).

	register uintptr_t *dispatch_table = ...

In order to update the pointer we would first of all need to remove the register specifier,
then we need to may it a static global, otherwise we cant access it in 'notice_safepoints'.
For good measure we need to declare it volatile to make sure the compiler cant do anything
clever. Yeuch (remember this pointer is accessed for every single bytecode).

I believe the above is thread safe.

In 'notice_safepoints' the VM state is 'synchronizing'. On return from 'notice_safepoints'
the VM state is changed to 'synchronized' (provided this was the last thread to synchronise).

Until the VM state is 'synchronized', the VM cannot make any assumptions as to whether
the interpreter is holding pointer to objects in registers or local storage.

Therefore unsafe operations such as GC are prohibited until the VM state is 'synchronized'.

During execution of 'notice_safepoints' half the handlers may point to safe handlers, half
may point to unsafe handlers. However, this will not cause any problems. The safe and
unsafe handlers are compatible in operation, its just the unsafe handlers dont call
'SafepointSynchronize::block(THREAD)'.

Now assume there is a pre-emptive thread swap while in the middle of 'notice_safepoints'.
The new thread then tries an unsafe operation (eg a GC request) which results in
'notice_safepoints' being called again.

The result is that the table may end up getting updated twice. In the really paranoid case
every single thread may be sitting in 'notice_safepoints'.

notice_safepoints() {
/* <- thread swap occurs here */
}

This is no different from the case where 'notice_safepoints' actually updates the table.
The VM is still not synchronised, and we have to either wait until control returns or
until someone else calls notice_safepoints. Remember the state is not set to synchronised
until all threads have synchronised.

On exit, before calling ignore_safepoints the VM is places in an unsynchronised state.
Therefore it doesn't matter whether we call the safe or unsafe branch handler. The
safe branch handler simply does an extra check on 'is_synchronizing' which returns false.

The only thing that could really ruin your day is if word write is not atomic, and I think
there is a lot of code that will break is word write is not atomic.

>
>> Finally, can someone enlighten me as to the purpose of
>> INCR_INVOCATION_COUNT, if we can get rid of that we could get rid of
>all
>> backedge checks.
>
>Surely the compile broker uses that count.

Yes, is the compiler the only thing that is using this? Is it used for profiling, or ...

We need to move to a world where we are not doing all these redundant checks with all the
attendant crud when the check trips to a world where we have a simple VM which just
handles the common default case.

If someone does require all this crud then we simply direct them to

BytecodeInterpreter::run_with_all_unnecessary_crud(..)

Regards,
Ed