Enabled LTO (and whitespace fixes) #867

Avamander · 2021-12-05T20:15:02Z

Basically, the compiler we're using has a bug that makes LTO not work properly with weak symbols.

It's more reasonable to simply upgrade.

Before:

Memory region         Used Size  Region Size  %age Used
           FLASH:      394628 B       480 KB     80.29%
             RAM:       54768 B        64 KB     83.57%

After:

Memory region         Used Size  Region Size  %age Used
           FLASH:      371812 B       480 KB     75.65%
             RAM:       54760 B        64 KB     83.56%

Note: Recovery build has not been tested, pinetime-app, pinetime-mcuboot-app seem to work.

evergreen22 · 2021-12-07T04:04:46Z

Does this PR close or obsolete #495 ?

Avamander · 2021-12-07T07:48:38Z

I don't think so.

Avamander · 2021-12-07T16:21:02Z

I see a lot of potential to further improve the set of optimization flags and the CMakefile (as done in #495).

However, I don't want to do it with this PR, it is already large as it is and pr-rot will soon get it unless merged.

.github/workflows/main.yml

Avamander · 2021-12-12T12:59:26Z

@geekbozu If you could take a look if everything seems fine, that'd be nice.

Squashed commit of the following: commit 68ad34d Author: Avamander <[email protected]> Date: Sun Dec 12 14:55:18 2021 +0200 Improved COMMON_FLAGS commit f002bce Author: Avamander <[email protected]> Date: Sun Dec 5 22:13:15 2021 +0200 Enabled LTO and various whitespace fixes

Avamander · 2021-12-14T10:16:17Z

@FintasticMan on Discord reported they're getting freezes. Investigating.

…blems.

JF002 · 2021-12-27T19:20:33Z

It appears that FreeRTOS heap and the stack of the main task (systemtask) are probably overflown.
I increased the size of the heap (18*1024), the size of the stack of SystemTask and disabled the HeartRate task (to free a bit of RAM) in this DFU :
pinetime-mcuboot-app-dfu-1.7.98.zip
This DFU also has RTT logging enabled, btw.

Patch : Test_LTO.txt

I'm running it right now on my sealed device, no issue to report right now.

When trying this branch, I also noticed a few issue, mainly with the Docker build : The cmake version embedded in the container is too old and cannot build the project and this URL (

InfiniTime/docker/build.sh

Line 42 in cb5fefc

    
           wget -q https://developer.arm.com/-/media/Files/downloads/gnu-rm/9-2020q2/$GCC_SRC -O - | tar -xj -C $TOOLS_DIR/

) does not exist.

ManuelBoe14 · 2021-12-28T00:12:52Z

I'm running it right now on my sealed device, no issue to report right now.

I have also installed version 1.7.98 on my Pinetime.
So far everything seems to be working.

trman · 2021-12-29T20:03:00Z

i installed 1.7.98 and the hr data no longer work (i need it for fitotrack)
no improvement in speed or battery whatsoever , on other part of infinitme (except the now no functional heat rate)
the dfu is near the limit of the pinetime too

do you plan to put back , the Heart rate task, before merging this pr for 1.8.0 release @JF002 ?

if anything , and if it's truly needed to remove a task , it might be better to remove something else than a core functionality of the pinetime
( the core functionality a the functionality that have a physical counterpart : steps (step counter hw ) / hr (green led HW)).

i see that the 1.7.98 is far bigger than usual ....

do that mean that enabling lto will forbid to add new functionality (like the near completed calculator of @Raupinger )?

for al i see , and if this the case this pr should not been merged until it doesn't need to remove functionality and doesn't take more space for no visible /perceptible improvement .

if the pr were to be merge as is by removing functionality and making impossible to add new one with removing existing app
it would be more a big regression than an improvement!!

by the way , i need to go in recovery in order to flash dfu because , in secured bluetooth , i can't no longer do a dfu update ....
is it related of this pr ? if not it shoiuld be resolved as well before 1.8.0
because , i don't think user will like no be able to update dfu ....

trman · 2021-12-29T21:12:04Z

i installed 1.78 and the hr data no longer work

@trman version 1.7.98 is for testing purposes only. JF002 has noted in his post above that he has deactivated the hr.

i do understand this , but he removed it becaause otherwise the dfu would not work .....
that's why iw wish that he don't use this 'trick' to validate this pr .... because i would not be able to use fitotrack

Avamander · 2021-12-29T21:15:50Z

@trman As written above, HR task was disabled.

If you need HR, don't flash random test builds meant to demonstrate something wholly different. It is not meant for regular usage and wasn't labeled as such. Any qualms about a random test build are really not relevant here and such test builds shouldn't be taken as something that is going to be instantly released.

Changes made are not all also immediately perceptible to the user, or really should be, there are other things that matter here as well.

Any issues you have with DFU are not relevant to this PR. You should really be expecting rough corners on something unreleased, it is unreleased for a reason. If you actually discover a bug that hasn't been reported, neither under that feature's PR or in an issue, then open a new one for that single bug.

JF002 · 2021-12-30T12:24:13Z

@trman
I'll give a bit of context about that 1.7.98 version. This is really a test build I built to check a single assumption : does increasing the heap of FreeRTOS and the stack size of SystemTask solve our memory corruption issue. The only purpose of this test build was to check the stability of InfiniTime with LTO enabled and the increased RAM allocation. Nothing else.

You've probably noticed : I didn't create a Pull-Request with those changes, because I don't want them to be merged.

But, as I had to free a bit of RAM to increase the heap/stack, I had to disable the HR task so I can use the RAM memory allocated to this task for other purposes. Again, I did this only for the purpose of this test and I won't merge that in develop/master

do that mean that enabling lto will forbid to add new functionality (like the near completed calculator of @Raupinger )?

The FLASH and RAM memories are already almost full, and that's why I cannot merge as many PR as I would like : I want to keep the memory usage under control, and some of these PRs have a lot of impact on the memory usage. The goal of LTO is to reduce the binary size (FLASH usage) so we can include some more functionalities. But, for some reason, using LTO seems to increase the RAM usage, and we still have to understand why and see what we can do about this.

by the way , i need to go in recovery in order to flash dfu because , in secured bluetooth , i can't no longer do a dfu update ....
is it related of this pr ? if not it shoiuld be resolved as well before 1.8.0
because , i don't think user will like no be able to update dfu ....

This is not related to this PR, please see this

JF002 · 2021-12-30T16:09:17Z

Here's a TODO list so we know what must still be done in this PR:

Measure the gain in FLASH space
Analyze RAM memory usage, adapt FreeRTOS heap and tasks stack size so they don't overflow. Try to provide an explanation why the RAM usage increased when enabling LTO
Fix DEBUG build
Fix Docker build
Check Github Actions build
Check the build warnings returned by the compiler and fix them if relevant

JF002 · 2022-01-03T12:15:26Z

@Avamander The test build ran for more than 6 days on my sealed unit (until I need to reboot to test other features). So I think it's quite stable once the stacks/heaps are correctly set 👍

trman · 2022-02-21T19:34:51Z

any news on the remaining task?

AlexXZero · 2022-11-27T05:25:01Z

src/main.cpp

+ */
+static __attribute__((used)) void TaskSwitchDummy() {
+  vTaskSwitchContext();
+}


This solution looks like a dirty hack, I would suggest to do the following more clear hack instead:

port.c: void xPortPendSVHandler( void ) { ... - " bl vTaskSwitchContext \n" + " bl _vTaskSwitchContext \n" ... } +void __attribute__((used)) _vTaskSwitchContext(void) +{ + vTaskSwitchContext(); +}

Note: I found that this solution shouldn't add any overhead as vTaskSwitchContext will be inlined into _vTaskSwitchContext.

AlexXZero · 2022-11-27T05:26:23Z

src/main.cpp


-  for (;;) {
+  // Yes, this should never be called, never is here, but it MUST be kept.
+  TaskSwitchDummy();


In fact, this function will be called as it is placed before infinity loop!

AlexXZero · 2022-11-27T05:27:21Z

src/FreeRTOS/port.c

                    ::"i"(configKERNEL_INTERRUPT_PRIORITY  << (8 - configPRIO_BITS))
 #endif
-                );
+      " .ltorg                \n");


Should this line be added before #ifdef SOFTDEVICE_PRESENT?

Avamander added the needs testing Needs testing on a physical device label Dec 5, 2021

Avamander force-pushed the patch-lto branch from 0949cc3 to 9199a80 Compare December 6, 2021 19:55

Avamander added this to the 1.8.0 milestone Dec 6, 2021

Avamander force-pushed the patch-lto branch 21 times, most recently from ba05271 to abc8bfb Compare December 12, 2021 01:52

Enabled LTO and various whitespace fixes

f002bce

Avamander force-pushed the patch-lto branch from abc8bfb to f002bce Compare December 12, 2021 01:53

Avamander commented Dec 12, 2021

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

Improved COMMON_FLAGS

68ad34d

Avamander requested a review from geekbozu December 12, 2021 12:59

Avamander and others added 2 commits December 19, 2021 02:00

Improved NimBLE flags, fixed main task stack size and a few style pro…

444b4e4

…blems.

Fix GCC url in dockerfile.

cb5fefc

JF002 added the needs more work This PR needs more work label Dec 30, 2021

Avamander mentioned this pull request Dec 30, 2021

Fix build with gcc 10.1 toolchain #896

Closed

JF002 modified the milestones: 1.8.0, 1.9.0 Jan 4, 2022

JF002 modified the milestones: 1.9.0, 1.10.0 Apr 2, 2022

Riksu9000 removed this from the 1.10.0 milestone Jun 23, 2022

AlexXZero reviewed Nov 27, 2022

View reviewed changes

mark9064 mentioned this pull request Nov 23, 2024

LTO #2169

Draft

Uh oh!

Enabled LTO (and whitespace fixes) #867

Are you sure you want to change the base?

Enabled LTO (and whitespace fixes) #867

Uh oh!

Conversation

Avamander commented Dec 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evergreen22 commented Dec 7, 2021

Uh oh!

Avamander commented Dec 7, 2021

Uh oh!

Avamander commented Dec 7, 2021

Uh oh!

Uh oh!

Avamander commented Dec 12, 2021

Uh oh!

Avamander commented Dec 14, 2021

Uh oh!

JF002 commented Dec 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ManuelBoe14 commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trman commented Dec 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trman commented Dec 29, 2021

Uh oh!

Avamander commented Dec 29, 2021

Uh oh!

JF002 commented Dec 30, 2021

Uh oh!

JF002 commented Dec 30, 2021 • edited by Avamander Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JF002 commented Jan 3, 2022

Uh oh!

trman commented Feb 21, 2022

Uh oh!

AlexXZero Nov 27, 2022

Choose a reason for hiding this comment

Uh oh!

AlexXZero Nov 27, 2022

Choose a reason for hiding this comment

Uh oh!

AlexXZero Nov 27, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Avamander commented Dec 5, 2021 •

edited

Loading

JF002 commented Dec 27, 2021 •

edited

Loading

ManuelBoe14 commented Dec 28, 2021 •

edited

Loading

trman commented Dec 29, 2021 •

edited

Loading

JF002 commented Dec 30, 2021 •

edited by Avamander

Loading