bugGNU Octave - Bugs: bug #61030, [octave forge] (image) Don't use...

 
 

bug #61030: [octave forge] (image) Don't use internal function

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Wed 11 Aug 2021 05:03:06 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  avinoam
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 29 Dec 2021 06:07:04 AM UTC, comment #18: 


Fix comment #17:

Thanks, @Markus and @Hartmut.

Avinoam Kalma <avinoam>
Group Member
Wed 29 Dec 2021 05:27:22 AM UTC, comment #17: 

I have checked the performance after applying this patch, according to @Hartmut suggestion (comment #7).
Did not see any noticeable change. There was a negligible improvement in the second test.

The @Markus and @Hartmut.

Pushed:
http://hg.code.sf.net/p/octave/image/rev/a40d4da8ecd3

Closing as fixed

Avinoam Kalma <avinoam>
Group Member
Fri 17 Dec 2021 11:19:20 AM UTC, comment #16: 

Not exactly: there is a patch in the mxe-octave that fixes the problem, but it works only on Windows.
I hope I will be able to apply Markus' patch to image package during this week.

Avinoam Kalma <avinoam>
Group Member
Thu 16 Dec 2021 07:57:38 PM UTC, comment #15: 

If I understand this right, this patch should be included into the image package, and a new release of the image package should be made, before Octave 7.1. comes out. Otherwise the image package will not install under Octave 7. And the release process for Octave 7.1 has already been started. Or am I missing something here?

Hartmut <hardy>
Thu 16 Dec 2021 01:56:29 PM UTC, comment #14: 

@Avinoam: Could you review the latest patch (comment #13) from Andy and check if it still compiles well under several versions (see comment #5) of core Octave and the image package tests still all pass? Afterwards we could probably push it to the image repo.

Hartmut <hardy>
Mon 23 Aug 2021 07:59:16 AM UTC, comment #13: 

The configure test was always failing afaict. So, it never used (the more efficient) `xelem`.

The attached updated patch should be fixing that.

(file #51807)

Markus Mützel <mmuetzel>
Group administrator
Sun 22 Aug 2021 01:58:52 PM UTC, comment #12: 

Thanks, @Markus,
this patch compiles under 4.2.0

I will make some tests to check the patch.

Avinoam Kalma <avinoam>
Group Member
Sun 22 Aug 2021 12:33:14 PM UTC, comment #11: 

Thanks, @Markus, I will check the new patch

Avinoam Kalma <avinoam>
Group Member
Sun 22 Aug 2021 12:28:17 PM UTC, comment #10: 

The attached patch adds a configure check to test if the `dim_vector` class has an `xelem` member function. If it doesn't, it would fall back to the `elem` function (with the additional cost of boundary checking).

I haven't tested at all, so it might need some tinkering. If it works, it should define HAVE_OCTAVE_DIM_VECTOR_XELEM if that function exists.

I also don't know the coding style of the image package. I hope it is ok.

(file #51803)

Markus Mützel <mmuetzel>
Group administrator
Sun 22 Aug 2021 11:47:32 AM UTC, comment #9: 

On ver 4.4.1 the compilation is OK

Avinoam Kalma <avinoam>
Group Member
Sun 22 Aug 2021 11:35:35 AM UTC, comment #8: 

Thanks, @Hartmut.

testing on 4.2.0 I get


In file included from connectivity.cc:17:0:
connectivity.h: In static member function 'static void octave::image::connectivity::set_padding(cons
t dim_vector&, const dim_vector&, T&, const P&)':
connectivity.h:156:43: error: 'const class dim_vector' has no member named 'xelem'
   const octave_idx_type row_stride = size.xelem (0);
                                           ^


So, this fix will not work 4.2.0.
Should continue to support this version?

Avinoam Kalma <avinoam>
Group Member
Wed 18 Aug 2021 07:31:20 PM UTC, comment #7: 

For the (only nice to have) test of the excution speed, you could use something quite simple like this:


clear

tic
for nloop = 1:100
 test bwconncomp
endfor
time_bwconncomp = toc/nloop

tic
for nloop = 1:3
 test imreconstruct
endfor
time_imreconstruct = toc/nloop


Running this (probably only) in the current core Octave release version, one time with the current image package release, and a second time (on the very same pc) with the "remove jit" patch applied to the image package (and properly re-compiled) should be enough to be sure that there are no "dramatic" speed regressions. We could then conclude something like "the speed decrease in the relevant BISTs is no more than x% with this patch applied." I know that BISTs are not made for this, but it would be quite easy to do it like this.

Hartmut <hardy>
Wed 18 Aug 2021 04:22:36 AM UTC, comment #6: 

Thanks, @Hartmut for the detailed review.

Some clarification questions and answers:

  • Commit message: the bug report number and the file names of all touched source code files are already mentioned.


  • Markus Mützel is the author of this patch.


  • I will check compilation and BISTs in older versions.


  • Could you please recommend how to check the run-time?


Thank again

Avinoam Kalma <avinoam>
Group Member
Mon 16 Aug 2021 03:06:41 PM UTC, comment #5: 

I am not very fluent in C, but I would like to see the following formal changes, or have the following formal checks done and reported here, before pushing this patch:

  • improve the wording of the commit message of the patch, e.g. mention the bug report number, and list the file names of all touched source code files. All as usual.


  • name Markus Mützel as author of this patch in the commit (as long as we mostly just copy his code changes over to the image repo).


  • Please check that the image package still compiles in all core Octave versions that we currently still support. (I assume that it compiles fine in current dev Octave, since the Octave CI setups don't complain. But does it also compile fine in Octave version 4.2.0, the oldest Octave core version still supported by the image package?)


  • Please check that the corresponding BISTs all still pass (at least the BISTs for the three files touched, and at least for Octave 4.2.0. Probably also in Octave 6.3.0 and 5.2.0 and 4.4.1)


  • Ideally (but optionally) do a small check if there is a (significant) run-time increase with this code change. If yes, we probably cannot do anything about it, it would just be nice to know.


I have recently done a full OS re-installation on my laptop, and won't be able to do those tests on my own machine in a reasonable time frame.

Soon I will also be offline for a couple of weeks. But as soon as the above items are check, I would be happy if someone (Avinoam?) pushes this patch to the repo.

Hartmut <hardy>
Sat 14 Aug 2021 07:25:14 PM UTC, comment #4: 
Avinoam Kalma <avinoam>
Group Member
Fri 13 Aug 2021 05:07:16 PM UTC, comment #3: 

The to_jit appears in
connectivity.h
bwconncomp.cc
imreconstruct.cc

I see that the patch in comment #0 handle this, so I will make a
proper changeset.

Thanks.
 


Avinoam Kalma <avinoam>
Group Member
Thu 12 Aug 2021 06:23:28 AM UTC, comment #2: 

Thanks for letting us know, Markus. I'm adding people to cc for now.

Hartmut <hardy>
Wed 11 Aug 2021 06:11:21 PM UTC, comment #1: 

Forgot to add a link to the changeset that removed this function in Octave:
https://hg.savannah.gnu.org/hgweb/octave/rev/f254c302bb9c

Markus Mützel <mmuetzel>
Group administrator
Wed 11 Aug 2021 05:03:06 PM UTC, original submission:  

The image package is using the function `dim_vector::to_jit` which was marked for internal usage with a comment in the header file. That function will finally be removed in Octave 7.

Consider replacing that function by accessing elements with `dim_vector::xelem` instead. That function should be equally efficient already with a very low level of compiler optimization.

A possible patch is in MXE Octave here:
https://hg.octave.org/mxe-octave/file/93d20ff5d6a6/src/of-image-1-no-jit.patch

Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51807:  bug61030-v3.patch added by mmuetzel (6KiB - application/octet-stream)
file #51803:  bug61030-v2.patch added by mmuetzel (6KiB - application/octet-stream)
file #51778:  bug61030.cs added by avinoam (5KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nrjank (Updated the item)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by hardy
  • -email is unavailable- added by hardy
  • -email is unavailable- added by mmuetzel (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 group members can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-12-29 avinoam StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2021-10-08 nrjank StatusIn Progress Patch Submitted
    2021-08-23 mmuetzel Attached File- Added bug61030-v3.patch, #51807
    2021-08-22 mmuetzel Attached File- Added bug61030-v2.patch, #51803
    2021-08-18 avinoam StatusConfirmed In Progress
    2021-08-14 avinoam Attached File- Added bug61030.cs, #51778
    2021-08-13 avinoam StatusNone Confirmed
        Assigned toNone avinoam
    2021-08-12 hardy Carbon-Copy- Added avinoam
        Carbon-Copy- Added carandraug

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code