0024044: Performance improvements: Foundation Classes (math)

(0024866)
Roman Lygin (developer)
2013-06-25 21:43

The fix pushed to the repository

(0024869)
abv (manager)
2013-06-26 06:59

Defining size of array independently in CDL and GXX is potentially dangerous. I suggest that definition of constants N, M, and MN in GXX should be done via sizeof() of relevant class fields.

The same comment as to 0024042: if you have some numbers of performance increase this improvement gives in your case, please share.

(0024907)
Roman Lygin (developer)
2013-06-27 22:41

For performance gain - please refer to 0024042.

Regarding sizeof(), do you see any better implementation than replacing each occurrence with something like (sizeof(Buf) / sizeof(Buf[0])). Though I share the desire to avoid risk of discrepancy this change would undermine code clarity, so I am not sure this is better than an explicit comment in the code. But I will change to whatever you prefer.

(0025041)
abv (manager)
2013-07-12 07:13

> replacing each occurrence with something like (sizeof(Buf) / sizeof(Buf[0]))

Yes, this is what I had in mind. Please check the amended version in CR24044_1

(0025090)
Roman Lygin (developer)
2013-07-17 00:19

The amendment is fine. Thanks for making it and sorry for the delay - I thought you expected me to *make* the amendment not just to *review* it.

(0025114)
apn (administrator)
2013-07-18 16:21

Dear BugMaster,

Branch CR24044_1 (and products from GIT master) was compiled on Linux and Windows platforms and tested.
SHA-1: fdbd14f0acc8325642ec1fd0553de0611f7fd57f

Number of compiler warnings:

occt component :
Linux: 2 (2 on master)
Windows: 1767 (1767 on master)

products component :
Linux: 0 (0 on master)
Windows: 288 (288 on master)

Regressions:
bugs moddata_2 bug524
http://occt-tests/CR24044-1-master-occt/Debian60-64/summary.html [^]

Improvements:
No improvements

Testing cases:
Not needed

Testing on Linux:
Total MEMORY difference: 366693152 / 365661156
Total CPU difference: 43746.34000000115 / 44445.42000000096

Testing on Windows:
Total MEMORY difference: 427509152 / 426010604
Total CPU difference: 43613.15625 / 31424.5

There are not differences in images found by testdiff.

(0025320)
mkv (tester)
2013-08-15 15:04

Dear BugMaster,
The fix should be integrated along with 0024045

Related Changesets
occt: master b28e7cc6 Timestamp: 2013-06-25 17:42:25 Author: Roman Lygin Committer: bugmaster [ Details ] [ Diff ]	0024044: Performance improvements: Foundation Classes (math)
	mod - src/math/math_DoubleTab.cdl	[ Diff ] [ File ]
	mod - src/math/math_DoubleTab.gxx	[ Diff ] [ File ]
	mod - src/math/math_SingleTab.cdl	[ Diff ] [ File ]
	mod - src/math/math_SingleTab.gxx	[ Diff ] [ File ]

Issue History
Date Modified	Username	Field	Change
2013-06-25 21:38	Roman Lygin	New Issue
2013-06-25 21:38	Roman Lygin	Assigned To	=> abv
2013-06-25 21:43	Roman Lygin	Note Added: 0024866
2013-06-25 21:43	Roman Lygin	Status	new => resolved
2013-06-25 21:49	Roman Lygin	Relationship added	child of 0024045
2013-06-25 22:00	Roman Lygin	Relationship added	related to 0024042
2013-06-26 06:59	abv	Note Added: 0024869
2013-06-26 06:59	abv	Assigned To	abv => Roman Lygin
2013-06-26 06:59	abv	Status	resolved => assigned
2013-06-27 22:41	Roman Lygin	Note Added: 0024907
2013-06-27 22:41	Roman Lygin	Assigned To	Roman Lygin => abv
2013-06-27 22:41	Roman Lygin	Status	assigned => feedback
2013-07-12 07:13	abv	Note Added: 0025041
2013-07-12 07:13	abv	Assigned To	abv => Roman Lygin
2013-07-12 07:13	abv	Status	feedback => resolved
2013-07-17 00:19	Roman Lygin	Note Added: 0025090
2013-07-17 00:19	Roman Lygin	Assigned To	Roman Lygin => abv
2013-07-17 00:19	Roman Lygin	Status	resolved => reviewed
2013-07-18 16:21	apn	Note Added: 0025114
2013-07-18 16:21	apn	Test case number	=> Not needed
2013-07-18 16:21	apn	Assigned To	abv => Roman Lygin
2013-07-18 16:21	apn	Status	reviewed => assigned
2013-07-18 16:21	apn	Target Version	=> 6.7.0
2013-08-15 15:04	mkv	Note Added: 0025320
2013-08-15 15:06	mkv	Assigned To	Roman Lygin => bugmaster
2013-08-15 15:06	mkv	Status	assigned => resolved
2013-08-15 15:06	mkv	Status	resolved => reviewed
2013-08-15 15:06	mkv	Status	reviewed => tested
2013-08-17 12:50	bugmaster	Changeset attached	=> occt master b28e7cc6
2013-08-17 12:50	bugmaster	Status	tested => verified
2013-08-17 12:50	bugmaster	Resolution	open => fixed
2013-12-19 13:52	bugmaster	Status	verified => closed
2013-12-19 13:56	bugmaster	Fixed in Version	=> 6.7.0
2015-03-09 19:36	msv	Relationship added	related to 0025746
2019-03-12 06:55	kgv	Relationship added	parent of 0030552

Notes
(0024866) Roman Lygin (developer) 2013-06-25 21:43	The fix pushed to the repository

(0024869) abv (manager) 2013-06-26 06:59	Defining size of array independently in CDL and GXX is potentially dangerous. I suggest that definition of constants N, M, and MN in GXX should be done via sizeof() of relevant class fields. The same comment as to 0024042: if you have some numbers of performance increase this improvement gives in your case, please share.

(0024907) Roman Lygin (developer) 2013-06-27 22:41	For performance gain - please refer to 0024042. Regarding sizeof(), do you see any better implementation than replacing each occurrence with something like (sizeof(Buf) / sizeof(Buf[0])). Though I share the desire to avoid risk of discrepancy this change would undermine code clarity, so I am not sure this is better than an explicit comment in the code. But I will change to whatever you prefer.

(0025041) abv (manager) 2013-07-12 07:13	> replacing each occurrence with something like (sizeof(Buf) / sizeof(Buf[0])) Yes, this is what I had in mind. Please check the amended version in CR24044_1

(0025090) Roman Lygin (developer) 2013-07-17 00:19	The amendment is fine. Thanks for making it and sorry for the delay - I thought you expected me to make the amendment not just to review it.

(0025114) apn (administrator) 2013-07-18 16:21	Dear BugMaster, Branch CR24044_1 (and products from GIT master) was compiled on Linux and Windows platforms and tested. SHA-1: fdbd14f0acc8325642ec1fd0553de0611f7fd57f Number of compiler warnings: occt component : Linux: 2 (2 on master) Windows: 1767 (1767 on master) products component : Linux: 0 (0 on master) Windows: 288 (288 on master) Regressions: bugs moddata_2 bug524 http://occt-tests/CR24044-1-master-occt/Debian60-64/summary.html [^] Improvements: No improvements Testing cases: Not needed Testing on Linux: Total MEMORY difference: 366693152 / 365661156 Total CPU difference: 43746.34000000115 / 44445.42000000096 Testing on Windows: Total MEMORY difference: 427509152 / 426010604 Total CPU difference: 43613.15625 / 31424.5 There are not differences in images found by testdiff.

(0025320) mkv (tester) 2013-08-15 15:04	Dear BugMaster, The fix should be integrated along with 0024045