patchGNU Octave - Patches: patch #7976, Make PPKG_ADD and PKG_DEL see both...

 
 

patch #7976: Make PPKG_ADD and PKG_DEL see both m-file and oct-file directories of a package (2nd post)

Submitted by:  Olaf Till <i7tiol>
Submitted on:  Wed 20 Mar 2013 11:28:22 PM UTC  
 
Category: NonePriority: 5 - Normal
Status: In ProgressPrivacy: Public
Assigned to: Jordi Gutiérrez Hermoso <jordigh>Open/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 22 Mar 2013 09:22:44 PM UTC, comment #7:

The reallocs are actually slow enough, slower than std::list, but the difference only matters for some very giant datasets:

http://www.baptiste-wicht.com/2012/11/cpp-benchmark-vector-vs-list/

Like I said originally, I agree with you, I don't think this really matters too much.

jwe said he wanted to look over your patch, so when he oks it, I'll push it, if he doesn't do it himself first.

Jordi Gutiérrez Hermoso <jordigh>
Project AdministratorIn charge of this item.
Fri 22 Mar 2013 09:11:19 PM UTC, comment #6:

They reallocate, but contrary to lists they preallocate space for many elements at once, so new (re)allocations are seldom needed. This makes them more efficient than lists in adding elements to the end, at least this is how I understand an explanation in http://www.cplusplus.com.

I don't think it's important enough in this case to profile which is better. If you believe otherwise, feel free to change to std::list.

Olaf Till <i7tiol>
Fri 22 Mar 2013 07:52:58 PM UTC, comment #5:

Adding elements to the end of an std::vector is precisely the situation for which std::vector is worse than std::list, since this requires reallocs.

Jordi Gutiérrez Hermoso <jordigh>
Project AdministratorIn charge of this item.
Fri 22 Mar 2013 07:51:12 PM UTC, comment #4:

I have revised the patch without applying the suggestion of the last post. Changes (suggested by Jordi):

- merged both patches

- shortened first line of commit message

- corrected the typos

- deleted commented out code

I did not use typedefs for the long iterator names since all over the file there are already similar names without typedefs. The choice of std::vector was commented in the last post.

(file #27661)

Olaf Till <i7tiol>
Fri 22 Mar 2013 07:40:15 AM UTC, comment #3:

As for the architecture issue, as I see it for the case of adding paths:

1st level: functions (without '::do_') defined in load-path.h, taking care only of instantiation;

2nd level: most functions with '::do_' in load-path.cc;

3rd level: ::do_add () in load-path.cc.

3rd level does actual path adding. But it also calls the add_hook.

2nd level calls 3rd level for path adding. But some functions in 2nd level want to call add_hook in their own way. So they have to temporarily set add_hook to zero and protect it while calling 3rd level. Also, some functions in 2nd level call 3rd level indirectly over a further 2nd level function (which has not much to do).

Suggestion: Let all 2nd level functions call 3rd level directly. Don't let 3rd level call the add_hook. This spares disabling it in the 2nd level. Let all 2nd level functions call the add_hook themselves; the only function which currently doesn't is ::do_append.

For the case of removing paths it's similar, but in one case 1st level ('remove()') calles 3rd level (::do_remove) directly. So for a change analog to adding paths one would have to introduce a short new function for the 2nd level here.

As for the choice of std::vector, it was because elements are only added to the end, which should be more efficient than with std::list.

Olaf Till <i7tiol>
Thu 21 Mar 2013 05:21:14 PM UTC, comment #2:

jordigh wrote:

I'm not entirely happy with the whole architecture of this patch, but I'm not entirely happy with the organisation of this whole load-path architecture either. It feels to me like we're adding wrapper functions on top of wrapper functions without fixing the actual underlying architectural problem.

What do you think the underlying problems are?

Maybe we should discuss this on the maintainers list before making changes? At least we should agree on what load-path should do and determine what it is not currently doing correctly.

John W. Eaton <jwe>
Project Administrator
Thu 21 Mar 2013 05:10:00 PM UTC, comment #1:

Your two csets depend on each other. Indeed, you'll see that the parent of pkg-addpath-rmpath.changeset is addpath-rmpath.changeset. In situations like these, it's best if you concatenate both into the same file. Here "hg export -r -2:-1" would do it, assuming that -2 and -1 point to these two commits (i.e. if they're the last two chronologically on your repository).

Your commit message for addpath-rmpath.changset has long lines. The first line should be short, under 80 characters.

Typo:

"direcories"?

We have some typedefs for those long iterator names, but why are you using std::vector instead of std::list? You don't need random access on this directory list. For the usual sizes of these lists, this difference in the choice of container doesn't matter much, granted.

Why are you leaving commented-out code? If the code needs to be deleted, banish it to VCS history, not to a comment.

I'm not entirely happy with the whole architecture of this patch, but I'm not entirely happy with the organisation of this whole load-path architecture either. It feels to me like we're adding wrapper functions on top of wrapper functions without fixing the actual underlying architectural problem.

Your second patch looks good. Small typo:

"ore"?

I recommend using Mercurial's histedit extension for polishing these two patches. Briefly, after enabling the histedit extension, do

and this should open an editor where you can specify what to do to your last two revisions. Select "edit" for both of them, close the editor, and edit each revision in turn, doing

when you're done with each one.

Jordi Gutiérrez Hermoso <jordigh>
Project AdministratorIn charge of this item.
Wed 20 Mar 2013 11:28:22 PM UTC, original submission:

sorry, in previous post I accidentally hit Enter

Problem:

PKG_ADD and PKG_DEL instructions may need access to both the architecture-dependent and the architecture-independent directories of a package. But as yet they are added to or removed from the path sequentielly, thereby separately calling PKG_ADD or PKG_DEL, respectively. In 'pkg build' they were even (added/removed) separately for each directory by generate_lookfor_cache.m/gen_doc_cache.m.

This recently caused problems for the database package in 'pkg build'. Reportedly there were also problems with other packages due to it. These problems have been worked around in the packages. But the fix in Octave is not difficult:

The first patch slightly modifies load-path.cc/h to make 'addpath' call the hook for the PKG_ADDs only after all argument directories are added (similarly as the ::do_set method already did it), and correspondingly to 'rmpath'.

The second patch changes some functions called by pkg.m to make use of the feature in the first path by always calling addpath and rmpath once for all directories.

I made a testpackage with PKG_ADD and PKG_DEL instructions as described above which yielded errors for 'pkg build', 'pkg unload', and 'pkg uninstall', and no errors with the patches. (testpackage not attached)

Olaf

Olaf Till <i7tiol>

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #27661:  revised-addpath-rmpath.changeset added by i7tiol (14kB - application/octet-stream)
file #27643:  addpath-rmpath.changeset added by i7tiol (7kB - application/octet-stream)
file #27644:  pkg-addpath-rmpath.changeset added by i7tiol (7kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by jordigh (Posted a comment)
  • -unavailable- added by i7tiol (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 22 Mar 2013 07:51:12 PM UTCi7tiolAttached File-=>Added revised-addpath-rmpath.changeset, #27661
    Thu 21 Mar 2013 05:10:00 PM UTCjordighStatusNone=>In Progress
      Assigned toNone=>jordigh
    Wed 20 Mar 2013 11:28:22 PM UTCi7tiolAttached File-=>Added addpath-rmpath.changeset, #27643
      Attached File-=>Added pkg-addpath-rmpath.changeset, #27644

    Back to the top


    Powered by Savane 3.1-cleanup