Smalltalk › Gnu

[PATCH] Process

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

7 messages Options

Gwenaël Casaccio

[PATCH] Process

Hi,

Here is a work in progress patch related to processes.
-I fix what I consider as an issue : while waiting on a semaphore
the process cannot be resumed (with resume but primResume can do it),
- the process creation is updated I didn't like the idea to yield the
current process
if a suspended process is created, the priority is correctly set no
need to change
it
- setting priority will put the process in the right queue.

I've added much more tests, hope the patch can be reviewed.

Gwen

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

0001-Update-process-creation-process-resume-does-not-resu.patch (24K) Download Attachment

Holger Freyther

Re: [PATCH] Process

On Mon, Mar 24, 2014 at 09:22:19AM +0100, Gwenaël Casaccio wrote:
> + Termination isNil ifFalse: [ ^ Termination ].
> + ^ [
> + Termination isNil ifTrue: [ Termination := MethodContext stack: 4 flags: 6 method: UndefinedObject>>#__terminate ip: 0 sp: -1 ].
> + Termination
> + ] valueWithoutPreemption
> + ]

^Termination ifNil: [
Termination := ..
].

?

> - makeUntrusted: aBoolean [
> - "Set whether the receiver is trusted or not."
> -
> - <category: 'basic'>
> - | ctx |
> - ctx := self context.
> - [ctx isNil] whileFalse:
> - [ctx makeUntrusted: aBoolean.
> - ctx := ctx parentContext]
> - ]

let's remove this separately and rhight now? Care to send a patch?

> - self suspend
> + self suspend.

unrelated patch. Please use git add -p and aim for minimal patches.

> - | activePriority |
> - activePriority := Processor activePriority.
> + | oldPriority |
> + oldPriority := priority.

tabs vs. spaces? Separate patch/fix as well.

> priority := anInteger.
> "Atomically move the process to the right list, preempting
> the current process if necessary."
> - self isReady ifTrue: [self resume].
> + (myList == (Processor processesAt: oldPriority)) ifTrue: [self primResume: false].

Okay. So this has always been broken. Do you want to create a separate
patch with the old self resume for it?

> - [aBlock on: ProcessBeingTerminated
> + [aBlock on: SystemExceptions.ProcessBeingTerminated

Why is that?

> - startExecution: aDirectedMessage [

Is this code dead? Can we remove it right now?

> onBlock: aBlockClosure at: aPriority suspend: aBoolean [
> <category: 'private'>
> - "It is important to retrieve this before we start the
> - process, because we want to choose whether to continue
> - running the new process based on the *old* activePriority,
> - not the one of the new process which is the maximum one."
> -
> - | closure activePriority |
> - activePriority := Processor activePriority.
> - closure :=
> - [[[
> - "#priority: is inlined for two reasons. First, to be able to
> - suspend the process, and second because we need to invert
> - the test on activePriority! This because here we may want to
> - yield to the creator, while in #priority: we may want to yield
> - to the process whose priority was changed."
> - priority := aPriority.
> - aBoolean
> - ifTrue: [self suspend]
> - ifFalse: [
> - aPriority < activePriority ifTrue: [ Processor yield ] ].
> - aBlockClosure value]
> - on: SystemExceptions.ProcessBeingTerminated
> - do:
> - [:sig |
> - "If we terminate in the handler, the 'ensure' blocks are not
> - evaluated. Instead, if the handler returns, the unwinding
> - is done properly."
> -
> - sig return]]
> - ensure: [self primTerminate]].
> -
> - "Start the Process immediately so that we get into the
> - #on:do: handler. Otherwise, we will not be able to
> - terminate the process with #terminate. The #resume will
> - preempt the forking process."
> - suspendedContext := closure asContext: nil.
> - priority := Processor unpreemptedPriority.
> - self
> - addToBeFinalized;
> - resume
> +
> + | closure |
> + closure := [ [ aBlockClosure value ] ensure: [ self primTerminate ] ].
> + suspendedContext := closure asContext: (self class termination) copy.
> + priority := aPriority.
> + self addToBeFinalized.
> + aBoolean ifTrue: [ ^ self ].
> + self primResume: false

tabs vs. spaces. And are you sure we can really simplify it that much? I need to
have a closer look ath this essential part.

> +TestCase subclass: TestProcess [

> + self assert: p_1 isSuspended.
> + 1 to: 9 do: [ :i | self assert: ((Processor processesAt: i) includes: p_1) not ].

Can we have Process return the interval of valid priorities.

> +
> + p_1 := [
> + ] fork.
> +
> + self assert: p_1 isTerminated.

hmm. I don't think test will be quite flaky in terms of scheduling and other
parts. Do you think we can mock/test this in a more reliable way?

> +
> + ok := #test_creation.
> + p_1 := [
> + ok := #p_13.
> + ] forkAt: 2.
> +
> + self assert: ok = #test_creation.

same here and probably the other tests.

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Gwenaël Casaccio

Re: [PATCH] Process

On 24/03/2014 10:50, Holger Hans Peter Freyther wrote:

> On Mon, Mar 24, 2014 at 09:22:19AM +0100, Gwenaël Casaccio wrote:
>> + Termination isNil ifFalse: [ ^ Termination ].
>> + ^ [
>> + Termination isNil ifTrue: [ Termination := MethodContext stack: 4 flags: 6 method: UndefinedObject>>#__terminate ip: 0 sp: -1 ].
>> + Termination
>> + ] valueWithoutPreemption
>> + ]
>
> ^Termination ifNil: [
> Termination := ..
> ].
>
> ?
>
>> - makeUntrusted: aBoolean [
>> - "Set whether the receiver is trusted or not."
>> -
>> - <category: 'basic'>
>> - | ctx |
>> - ctx := self context.
>> - [ctx isNil] whileFalse:
>> - [ctx makeUntrusted: aBoolean.
>> - ctx := ctx parentContext]
>> - ]
> let's remove this separately and rhight now? Care to send a patch?
>
>> - self suspend
>> + self suspend.
> unrelated patch. Please use git add -p and aim for minimal patches.
>
>> - | activePriority |
>> - activePriority := Processor activePriority.
>> + | oldPriority |
>> + oldPriority := priority.
> tabs vs. spaces? Separate patch/fix as well.
>
>
>> priority := anInteger.
>> "Atomically move the process to the right list, preempting
>> the current process if necessary."
>> - self isReady ifTrue: [self resume].
>> + (myList == (Processor processesAt: oldPriority)) ifTrue: [self primResume: false].
> Okay. So this has always been broken. Do you want to create a separate
> patch with the old self resume for it?
>
>> - [aBlock on: ProcessBeingTerminated
>> + [aBlock on: SystemExceptions.ProcessBeingTerminated

ProcessBeingTerminated is in SystemExceptions namespace otherwhise it's nil

> Why is that?
>
>> - startExecution: aDirectedMessage [
> Is this code dead? Can we remove it right now?
It's dead

>> onBlock: aBlockClosure at: aPriority suspend: aBoolean [
>> <category: 'private'>
>> - "It is important to retrieve this before we start the
>> - process, because we want to choose whether to continue
>> - running the new process based on the *old* activePriority,
>> - not the one of the new process which is the maximum one."
>> -
>> - | closure activePriority |
>> - activePriority := Processor activePriority.
>> - closure :=
>> - [[[
>> - "#priority: is inlined for two reasons. First, to be able to
>> - suspend the process, and second because we need to invert
>> - the test on activePriority! This because here we may want to
>> - yield to the creator, while in #priority: we may want to yield
>> - to the process whose priority was changed."
>> - priority := aPriority.
>> - aBoolean
>> - ifTrue: [self suspend]
>> - ifFalse: [
>> - aPriority < activePriority ifTrue: [ Processor yield ] ].
>> - aBlockClosure value]
>> - on: SystemExceptions.ProcessBeingTerminated
>> - do:
>> - [:sig |
>> - "If we terminate in the handler, the 'ensure' blocks are not
>> - evaluated. Instead, if the handler returns, the unwinding
>> - is done properly."
>> -
>> - sig return]]
>> - ensure: [self primTerminate]].
>> -
>> - "Start the Process immediately so that we get into the
>> - #on:do: handler. Otherwise, we will not be able to
>> - terminate the process with #terminate. The #resume will
>> - preempt the forking process."
>> - suspendedContext := closure asContext: nil.
>> - priority := Processor unpreemptedPriority.
>> - self
>> - addToBeFinalized;
>> - resume
>> +
>> + | closure |
>> + closure := [ [ aBlockClosure value ] ensure: [ self primTerminate ] ].
>> + suspendedContext := closure asContext: (self class termination) copy.
>> + priority := aPriority.
>> + self addToBeFinalized.
>> + aBoolean ifTrue: [ ^ self ].
>> + self primResume: false
> tabs vs. spaces. And are you sure we can really simplify it that much? I need to
> have a closer look ath this essential part.

Yes also for another reason with my change the process is always on the
right priority queue,
the current implementation would only put it when it's yielding or
scheduled. I strongly
believe that the invariant : ((processor processesAt: priority) select:
self) should be correct.

>> +TestCase subclass: TestProcess [
>> + self assert: p_1 isSuspended.
>> + 1 to: 9 do: [ :i | self assert: ((Processor processesAt: i) includes: p_1) not ].
> Can we have Process return the interval of valid priorities.
>
>> +
>> + p_1 := [
>> + ] fork.
>> +
>> + self assert: p_1 isTerminated.
> hmm. I don't think test will be quite flaky in terms of scheduling and other
> parts. Do you think we can mock/test this in a more reliable way?
>

I agree it only works when you've a clean image and this is what I want
otherwise
tests needs to rely on multiple process/scheduling parts that I want to
tests separately.
>> +
>> + ok := #test_creation.
>> + p_1 := [
>> + ok := #p_13.
>> + ] forkAt: 2.
>> +
>> + self assert: ok = #test_creation.
> same here and probably the other tests.
>

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Holger Freyther

Re: [PATCH] Process

On Mon, Mar 24, 2014 at 12:01:16PM +0100, Gwenaël Casaccio wrote:
>
> ProcessBeingTerminated is in SystemExceptions namespace otherwhise it's nil

oops. so this has never worked so far?

> Yes also for another reason with my change the process is always on
> the right priority queue,
> the current implementation would only put it when it's yielding or
> scheduled. I strongly
> believe that the invariant : ((processor processesAt: priority)
> select: self) should be correct.

yes. Could you make a separate patch to enforce this?

> >hmm. I don't think test will be quite flaky in terms of scheduling and other
> >parts. Do you think we can mock/test this in a more reliable way?
> >
> I agree it only works when you've a clean image and this is what I
> want otherwise
> tests needs to rely on multiple process/scheduling parts that I want
> to tests separately.

"clean" it means that the call-in this test is executed from needs
to have lower priority than the forked call. E.g. what happens if we
ever execute it from VisualGST or if we ever have multiple native
dispatchers?

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Paolo Bonzini-2

Re: [PATCH] Process

In reply to this post by Holger Freyther

Il 24/03/2014 10:50, Holger Hans Peter Freyther ha scritto:
>> > - startExecution: aDirectedMessage [
> Is this code dead? Can we remove it right now?
>

Yes:

2006-12-28 Paolo Bonzini <[hidden email]>

* libgst/interp.c: Remove commented out code to start call-ins through
Process>>#startExecution:.

but please remove all remnants in libgst/ as well.

I hardly understand any of the changes in this patch, which is
scary. While we have unit tests, you really need to explain
the changes and/or split them in multiple patches.

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Paolo Bonzini-2

Re: [PATCH] Process

In reply to this post by Holger Freyther

Il 24/03/2014 15:07, Holger Hans Peter Freyther ha scritto:
>> > Yes also for another reason with my change the process is always on
>> > the right priority queue, the current implementation would only put
>> > it when it's yielding or scheduled.

As opposed to what?

This invariant:

>> ((processor processesAt: priority) select: self)

is obviously wrong if the process is suspendend (myList := nil) or on a
semaphore.

Paolo

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk

Gwenaël Casaccio

Re: [PATCH] Process

In reply to this post by Paolo Bonzini-2

On 25/03/2014 16:15, Paolo Bonzini wrote:

> Il 24/03/2014 10:50, Holger Hans Peter Freyther ha scritto:
>>>> - startExecution: aDirectedMessage [
>> Is this code dead? Can we remove it right now?
>>
> Yes:
>
> 2006-12-28 Paolo Bonzini <[hidden email]>
>
> * libgst/interp.c: Remove commented out code to start call-ins through
> Process>>#startExecution:.
>
> but please remove all remnants in libgst/ as well.
>
> I hardly understand any of the changes in this patch, which is
> scary. While we have unit tests, you really need to explain
> the changes and/or split them in multiple patches.
>
> Paolo

This was not a patch but a work in progress in no way it should
have been included in that state.

Gwen

_______________________________________________
help-smalltalk mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/help-smalltalk