bugPSPP - Bugs: bug #20910, GUI becomes extremely slow after...

 
 

bug #20910: GUI becomes extremely slow after executing syntax which doesn't read the data

Submitter:  John Darrington <jmd>
Submitted:  Mon 27 Aug 2007 12:12:15 AM UTC
   
 
Category:  Graphical User Interface Severity:  7 - Major
Status:  Fixed Assigned to:  blp
Open/Closed:  Closed Release:  Before 0.6.0
Effort:  0.00
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 19 Sep 2007 04:29:44 AM UTC, comment #12: 


>This latest patch looks good. Let's check it in and close this item.


Done.  Thanks for all the great discussion and assistance on this one!

Ben Pfaff <blp>
Group administrator
Wed 19 Sep 2007 03:31:22 AM UTC, comment #11: 

I thought I replied to this a couple of days ago, but it seems not ...

This latest patch looks good.  Let's check it in and close this item.

John Darrington <jmd>
Group administrator
Mon 17 Sep 2007 03:51:16 AM UTC, comment #10: 


>What do you think of this approach?


It's good.  I don't know why I had mentally discarded this idea.  A serial number protocol works fine.

But: I didn't like the idea of adding a new field to every casereader for something so rare.  I tried hard to keep down the size of struct casereader to a bare minimum, because I don't want anyone to hesitate to use the casereader mechanism because of time or space overhead.  So, let me propose the -6 patch now attached (also an interdiff to show what changed) which avoids putting a serial number on every casereader.

Does this look OK to you?

(file #13964, file #13965)

Ben Pfaff <blp>
Group administrator
Sat 15 Sep 2007 05:50:18 AM UTC, comment #9: 

Here's a modified version of your patch (against the current head) which shows what I had in mind.

There's some ends which could be tidied up with this patch, and it seems to produce some GtkWarnings which didn't used to be there, but it seems to work, and so far as I can tell doesn't leak any memory.

What do you think of this approach?

(file #13954)

John Darrington <jmd>
Group administrator
Fri 14 Sep 2007 05:05:45 AM UTC, comment #8: 


>Couldn't the struct casereader contain a serial number ?


Sure.  Then execute_syntax needs to record the serial number before sticking the lazy casereader into the data set and compare the serial number of the data set's casereader against the one it recorded after running the syntax.  For the proper level of abstraction, we could encapsulate the serial number in a structure and provide a function to compare serial numbers.  If you call that structure "struct lazy_status", we're right back where we are now :-)

Seriously, a serial number protocol would work fine, but I don't see how it's an improvement.  Maybe you have a suggestion for how to do better with a serial number protocol.

Ben Pfaff <blp>
Group administrator
Thu 13 Sep 2007 05:32:19 AM UTC, comment #7: 

Couldn't the struct casereader contain a serial number ?


John Darrington <jmd>
Group administrator
Thu 13 Sep 2007 04:58:20 AM UTC, comment #6: 


>...Maybe just call it lazy_status_destroy...?


OK.  Patch updated.

>2. It would follow the OO paradigm more closely if there was a struct
>called struct lazy_casereader, which inhereted from an ordinary
>casereader. It would need to contain extra state (what's currently in
>struct lazy_status) and would need one extra method (currently
>performed by lazy_status_destroy).


In an earlier version of the patch (that I didn't post), lazy_status
was called lazy_casereader.  It still wasn't an actual casereader
though.  The renaming to lazy_status was the last thing I did before
posting the -4 patch.

I would like a lazy_casereader to be as much like a regular casereader
as I could make it.  Unfortunately I'm at a loss how to do a couple of
things.  First, I think there needs to be something like lazy_status
that is separate from the lazy casereader.  Why?  Because, otherwise,
when you put the lazy casereader into the data set, how do you know
whether you got the same one back after executing the syntax?  You
can't just check whether you got back a lazy casereader (there's no
reason to believe that other code couldn't put a lazy casereader into
the data set, especially since we're putting it in generic code in
src/data now).  You can't compare the "struct casereader *" you put in
against the one you got out (it could be the same address but the
casereader you put in was freed and then a new one malloc'd later got
the same address).  You can't clone the lazy casereader and then pass
in the clone, because cloning the casereader instantiates it.

I had at least one other problem to mention but that one seems
sufficient for now :-)  Do you have an idea?


(file #13932)

Ben Pfaff <blp>
Group administrator
Wed 12 Sep 2007 05:52:03 AM UTC, comment #5: 


> Is it what you had in mind?


More or less.  Here's some additional comments:

1. Consider renaming lazy_casereader_destroy.  It doesn't destroy the casereader. It (conditionally) destroys the status, (and returns some state from it).  Maybe just call it lazy_status_destroy or lazy_casereader_is_instantiated ?

2. It would follow the OO paradigm more closely if there was a struct called struct lazy_casereader, which inhereted from an ordinary casereader.  It would need to contain extra state (what's currently in struct lazy_status) and would need one extra method (currently performed by lazy_status_destroy).  That way, we don't need a seperate lazy_status at all, so would make execute_syntax simpler.  On the down side it's more work, and means exposing some of the casereader internals so perhaps it's not worth the effort.


John Darrington <jmd>
Group administrator
Wed 12 Sep 2007 04:41:38 AM UTC, comment #4: 

I think that the new patch that I'm attaching addresses your comments.  Is it what you had in mind?

(file #13922)

Ben Pfaff <blp>
Group administrator
Mon 10 Sep 2007 01:58:02 AM UTC, comment #3: 

The principle is quite elegant IMO and it seems to work too.

Some ideas which might make the implementation more elegant:

0. Make lazy_casereader a class derived from casereader ...

1. ... this means that used_reader and owns_reader can be methods    on lazy_casereader which inspect and mutate the object respectively.

2. lazy_casereader need not contain a PsppireDataStore --- it would be sufficient to contain a struct datasheet , which would mean that lazy casereader could live in src/data where is belongs.


John Darrington <jmd>
Group administrator
Sun 09 Sep 2007 06:11:25 PM UTC, comment #2: 

Here's a patch that fixes the problem for me.  I can't decide whether it's elegant or disgusting.

(file #13905)

Ben Pfaff <blp>
Group administrator
Tue 28 Aug 2007 04:27:14 AM UTC, comment #1: 


>I think Ben's already working on this.


Yes.  Thanks for filing a bug report as a reminder.

Ben Pfaff <blp>
Group administrator
Mon 27 Aug 2007 12:12:15 AM UTC, original submission:  

I think Ben's already working on this.  Submitting it here anyway for completeness' sake.

John Darrington <jmd>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #13964:  datasheet-speedup-6.patch added by blp (15KiB - text/x-diff)
file #13965:  datasheet-speedup-5-6.interdiff added by blp (12KiB - application/octet-stream)
file #13954:  my_lazy.patch added by jmd (17KiB - text/x-diff)
file #13932:  datasheet-speedup-5.patch added by blp (13KiB - text/x-diff)
file #13922:  datasheet-speedup-4.patch added by blp (13KiB - text/x-diff)
file #13905:  datasheet-speedup2.patch added by blp (8KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by blp (Posted a comment)
  • -email is unavailable- added by jmd (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-09-19 blp StatusReady for Test/Review Fixed
        Open/ClosedOpen Closed
    2007-09-17 blp Attached File- Added datasheet-speedup-6.patch, #13964
        Attached File- Added datasheet-speedup-5-6.interdiff, #13965
    2007-09-15 jmd Attached File- Added my_lazy.patch, #13954
    2007-09-13 blp Attached File- Added datasheet-speedup-5.patch, #13932
    2007-09-12 blp Attached File- Added datasheet-speedup-4.patch, #13922
    2007-09-09 blp Attached File- Added datasheet-speedup2.patch, #13905
        StatusIn Progress Ready for Test/Review
    2007-08-28 blp StatusNone In Progress
    2007-08-27 jmd ReleaseNone Before 0.6.0

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code