Hi Daniel,

On Fri, Aug 14, 2009 at 9:26 PM, Daniel A. Steffen <das@users.sourceforge.net> wrote:
Hi Youness,

On Fri, Aug 14, 2009 at 22:06, Daniel A.
Steffen<das@users.sourceforge.net> wrote:
> On Fri, Aug 14, 2009 at 19:26, Youness
> Alaoui<kakaroto@kakaroto.homelinux.net> wrote:
>> 1 - drawing widgets seems extremely slow compared to TkCarbon, try the
>> preferences window and change from one tab to another, you'll see it draw
>> each widget one at a time, while on Carbon, it was instantenuous.
>
> I haven't observed slowness

with the workaround for 2. below in place, I have now also observed
this as well.
It turns out to be related to the idle-time redrawing behaviour of Tk,
 the only items affected are widgets drawn via native NSViews, i.e.
push/check/radiobuttons, scrollbars and menubuttons.

At idle time when these are drawn, NSWindow flushing is enabled
(whereas in the ordinary AppKit draw loop it is disabled) and thus
their drawing is immediately flushed to screen.
To workaround this it suffices to disable/enable flushing for the
containing window in TkMacOSXSetup/RestoreDrawingContext(), c.f. patch
below (against de-carbon-8-5, but should apply to HEAD as well).
Note that the window is always flushed at the next passage through the
event loop anyway (via -tkDisplayIfNeeded), or upon explicit call to
[update] (via XSync()).

The patch avoids the issue and AFAICT doesn't appear to break
anything, but I would appreciate some wider testing before I commit.
Hi, I've tested it and seems to work just fine, the slowness compeltely disappeared now! Good job!
 


>> 2 - At one point, when I opened the preferences window, aMSN froze, I used
>> Command-, and the 'aMSN' menu stayed highlited, and it looked as if the
>> runloop had stopped because it didn't handle my mouse events anymore (focus
>> on windows, or the close/minimize/maximize buttons stopped getting
>> hovered..). It doesn't always happen, but it's still easily reproducable...
>
> I have seen that, breaking into the debugger at that point shows that
> this is caused by aMSN calling [update] or [vwait] at inopportune
> times (e.g. I have seen these called from an [after] event handler and
> from a channel notification handler), doing this causes recursive
> entry of the runloop at locations where Tk and AppKit do not expect
> this to happen...

I cannot do anything about the nested [vwait]s, but I have been able
to avoid the lockups from nested [update]s, c.f. patch to tcl notifier
below. It should only affect code running with the notifier in
embedded mode (i.e. probably only TkAqua Cocoa so far), certainly the
tcl test suite appears unaffected.

The fix consists mainly of making sure that event loops entered
recursively from the runloop observer's Tcl_ServiceAll() processing
(e.g. via some event handler calling [update]) run in the
tcl-events-only runloop mode, and that polling Tcl_WaitForEvent()'s
called at that time wake up the notifier thread in all cases.
The one tricky thing is that a second runloop observer is needed for
the tcl-events-only runloop mode (with the same observer handler as
the original), as it turns out that CFRunLoop prevents the handler of
a given observer from being entered recursively...

As this is a slightly more invasive change, I would appreciate if a
few people could do some testing and running of the tcl testsuite to
make sure this is not breaking something unexpected, I don't have time
to do extensive testing myself ATM...
I tested de-carbon-8-5 (to which you already committed these patches) and it worked nicely.I tried the preferences window a couple of times and it worked correctly, and I haven't experienced a freeze so far. Good job Daniel!
By the way, slightly off-topic, I've heard all the rumors about the [update] command to avoid, etc.. but it's a bit hard to do that, we use it a lot in a lot of cases, and in all cases, we had absolutely no choice... and considering that any function call in Tk is the result of an 'after' or a file event, or a UI interaction, etc.. it would be useless to have 'update' but not be able to use it in some obscure situations... 
About [vwait], I didn't know we used that... Would it make a difference to use [tkwait variable] or is that just the exact same thing? There are some use cases where we also have no choice but to use that command ([tkwait variable]), so...
In any case, so far so good, it seems to work fine, thanks!
Thanks again,
Youness.
 


Thanks!

Cheers,

Daniel

--
** Daniel A. Steffen                   **
** <mailto:das@categorifiedcoder.info> **


diff --git tk/macosx/tkMacOSXDraw.c tk/macosx/tkMacOSXDraw.c
index 83492e8..23dd70e 100644
--- tk/macosx/tkMacOSXDraw.c
+++ tk/macosx/tkMacOSXDraw.c
@@ -1565,6 +1565,7 @@ TkMacOSXSetupDrawingContext(
           if (dontDraw) {
               goto end;
           }
+           [[view window] disableFlushWindow];
           dc.view = view;
           dc.context = [[NSGraphicsContext currentContext] graphicsPort];
           dc.portBounds = NSRectToCGRect([view bounds]);
@@ -1690,6 +1691,7 @@ TkMacOSXRestoreDrawingContext(
 {
    if (dcPtr->context) {
       CGContextSynchronize(dcPtr->context);
+       [[dcPtr->view window] enableFlushWindow];
       if (dcPtr->focusLocked) {
           [dcPtr->view unlockFocus];
       } else {
diff --git tcl/macosx/tclMacOSXNotify.c tcl/macosx/tclMacOSXNotify.c
index 706c988..193a46e 100644
--- tcl/macosx/tclMacOSXNotify.c
+++ tcl/macosx/tclMacOSXNotify.c
@@ -246,6 +246,8 @@ typedef struct ThreadSpecificData {
                                * performed. */
    int runLoopRunning;                /* True if this thread's Tcl runLoop is running */
    int runLoopNestingLevel;   /* Level of nested runLoop invocations */
+    int runLoopServicingEvents;        /* True if this thread's runLoop is servicing
+                                * tcl events */
    /* Must hold the notifierLock before accessing the following fields: */
    /* Start notifierLock section */
    int onList;                        /* True if this thread is on the waitingList */
@@ -277,7 +279,7 @@ typedef struct ThreadSpecificData {
                               /* Any other thread alerts a notifier that an
                                * event is ready to be processed by signaling
                                * this CFRunLoopSource. */
-    CFRunLoopObserverRef runLoopObserver;
+    CFRunLoopObserverRef runLoopObserver, runLoopObserverTcl;
                               /* Adds/removes this thread from waitingList
                                * when the CFRunLoop starts/stops. */
    CFRunLoopTimerRef runLoopTimer;
@@ -453,7 +455,7 @@ Tcl_InitNotifier(void)
       CFRunLoopSourceRef runLoopSource;
       CFRunLoopSourceContext runLoopSourceContext;
       CFRunLoopObserverContext runLoopObserverContext;
-       CFRunLoopObserverRef runLoopObserver;
+       CFRunLoopObserverRef runLoopObserver, runLoopObserverTcl;

       bzero(&runLoopSourceContext, sizeof(CFRunLoopSourceContext));
       runLoopSourceContext.info = tsdPtr;
@@ -477,12 +479,21 @@ Tcl_InitNotifier(void)
                   "CFRunLoopObserver");
       }
       CFRunLoopAddObserver(runLoop, runLoopObserver, kCFRunLoopCommonModes);
-       CFRunLoopAddObserver(runLoop, runLoopObserver,
+       runLoopObserverTcl = CFRunLoopObserverCreate(NULL,
+               kCFRunLoopEntry|kCFRunLoopExit|kCFRunLoopBeforeWaiting, TRUE,
+               LONG_MIN, UpdateWaitingListAndServiceEvents,
+               &runLoopObserverContext);
+       if (!runLoopObserverTcl) {
+           Tcl_Panic("Tcl_InitNotifier: could not create "
+                   "CFRunLoopObserver");
+       }
+       CFRunLoopAddObserver(runLoop, runLoopObserverTcl,
               tclEventsOnlyRunLoopMode);

       tsdPtr->runLoop = runLoop;
       tsdPtr->runLoopSource = runLoopSource;
       tsdPtr->runLoopObserver = runLoopObserver;
+       tsdPtr->runLoopObserverTcl = runLoopObserverTcl;
       tsdPtr->runLoopTimer = NULL;
       tsdPtr->waitTime = CF_TIMEINTERVAL_FOREVER;
       tsdPtr->tsdLock = SPINLOCK_INIT;
@@ -710,6 +721,9 @@ Tcl_FinalizeNotifier(
       CFRunLoopObserverInvalidate(tsdPtr->runLoopObserver);
       CFRelease(tsdPtr->runLoopObserver);
       tsdPtr->runLoopObserver = NULL;
+       CFRunLoopObserverInvalidate(tsdPtr->runLoopObserverTcl);
+       CFRelease(tsdPtr->runLoopObserverTcl);
+       tsdPtr->runLoopObserverTcl = NULL;
       if (tsdPtr->runLoopTimer) {
           CFRunLoopTimerInvalidate(tsdPtr->runLoopTimer);
           CFRelease(tsdPtr->runLoopTimer);
@@ -1150,9 +1164,8 @@ int
 Tcl_WaitForEvent(
    Tcl_Time *timePtr)         /* Maximum block time, or NULL. */
 {
-    int result, polling;
+    int result, polling, runLoopRunning;
    CFTimeInterval waitTime;
-    CFStringRef runLoopMode;
    SInt32 runLoopStatus;
    ThreadSpecificData *tsdPtr;

@@ -1206,16 +1219,12 @@ Tcl_WaitForEvent(
     * added to the common run loop modes might get lost.
     */

-    if (tsdPtr->runLoopRunning) {
-       runLoopMode = tclEventsOnlyRunLoopMode;
-    } else {
-       runLoopMode = kCFRunLoopDefaultMode;
-       tsdPtr->runLoopRunning = 1;
-    }
-    runLoopStatus = CFRunLoopRunInMode(runLoopMode, waitTime, TRUE);
-    if (runLoopMode == kCFRunLoopDefaultMode) {
-       tsdPtr->runLoopRunning = 0;
-    }
+    runLoopRunning = tsdPtr->runLoopRunning;
+    tsdPtr->runLoopRunning = 1;
+    runLoopStatus = CFRunLoopRunInMode(tsdPtr->runLoopServicingEvents ||
+           runLoopRunning ? tclEventsOnlyRunLoopMode : kCFRunLoopDefaultMode,
+           waitTime, TRUE);
+    tsdPtr->runLoopRunning = runLoopRunning;

    LOCK_NOTIFIER_TSD;
    tsdPtr->polling = 0;
@@ -1333,19 +1342,22 @@ UpdateWaitingListAndServiceEvents(
 {
    ThreadSpecificData *tsdPtr = (ThreadSpecificData*) info;

+    if (tsdPtr->sleeping) {
+       return;
+    }
    switch (activity) {
    case kCFRunLoopEntry:
       tsdPtr->runLoopNestingLevel++;
-       if (tsdPtr->runLoopNestingLevel == 1 && !tsdPtr->sleeping &&
-               (tsdPtr->numFdBits > 0 || tsdPtr->polling)) {
+       if (tsdPtr->numFdBits > 0 || tsdPtr->polling) {
           LOCK_NOTIFIER;
-           OnOffWaitingList(tsdPtr, 1, 1);
+           if (!OnOffWaitingList(tsdPtr, 1, 1) && tsdPtr->polling) {
+              write(triggerPipe, "", 1);
+           }
           UNLOCK_NOTIFIER;
       }
       break;
    case kCFRunLoopExit:
-       if (tsdPtr->runLoopNestingLevel == 1 && !tsdPtr->sleeping &&
-               (tsdPtr->numFdBits > 0 || tsdPtr->polling)) {
+       if (tsdPtr->runLoopNestingLevel == 1) {
           LOCK_NOTIFIER;
           OnOffWaitingList(tsdPtr, 0, 1);
           UNLOCK_NOTIFIER;
@@ -1353,9 +1365,11 @@ UpdateWaitingListAndServiceEvents(
       tsdPtr->runLoopNestingLevel--;
       break;
    case kCFRunLoopBeforeWaiting:
-       if (!tsdPtr->sleeping && tsdPtr->runLoopTimer &&
+       if (tsdPtr->runLoopTimer && !tsdPtr->runLoopServicingEvents &&
               (tsdPtr->runLoopNestingLevel > 1 || !tsdPtr->runLoopRunning)) {
+           tsdPtr->runLoopServicingEvents = 1;
           while (Tcl_ServiceAll() && tsdPtr->waitTime == 0) {}
+           tsdPtr->runLoopServicingEvents = 0;
       }
       break;
    default: