Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Load timer #405

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Load timer #405

wants to merge 8 commits into from

Conversation

Tearran
Copy link
Member

@Tearran Tearran commented Feb 1, 2025

Description

refinement of #326

Added a Timer function to help identify sections that are slow and may need optimization or separation to improve performance.

./tools/armbian-config-dev and ./bin/armbian-config

Initializing script: 0 seconds
Setting base data: 0 seconds
The current Armbian 24.8.4 stable (bookworm) is supported.: 4 seconds
Loaded Runtime variables...: 0 seconds
Loaded Dialog...: 0 seconds
Loaded Docs...: 0 seconds
Loaded System helpers...: 0 seconds
Loaded Network helpers...: 0 seconds
Loaded Software helpers...: 0 seconds

note '--api' '--docs' and '! -t =$0" do not output timer results see

./bin/armbian-config --api set_checkpoint help

Usage: set_checkpoint <start|stop|mark|show> [description] [show]
Commands:
  start              Start the timer.
  stop               Stop the timer.
  mark [description] [time] Mark a checkpoint with an optional description and an optional flag to show the output.
  show               Show the total elapsed time and checkpoints.

Issue reference:
[Bug]: sanitize_input #324
function raceing
Partly addresses UX vs Non interactive mode use

Implementation Details

  • added variable checks for UX mode to help separate from the non interactive modes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have ensured that my changes do not introduce new warnings or errors
  • No new external dependencies are included
  • Changes have been tested and verified
  • I have included necessary metadata in the code, including associative arrays

@github-actions github-actions bot added the size/large PR with 250 lines or more label Feb 1, 2025
@Tearran Tearran marked this pull request as ready for review February 1, 2025 20:48
@dimitry-ishenko
Copy link
Collaborator

dimitry-ishenko commented Feb 2, 2025

@Tearran IMHO it would be PITA to maintain two copies of armbian-config.

What about using an env var (eg, ARMBIAN_CONFIG_DEBUG or just DEBUG) to show the timing info? Eg,

DEBUG=1 ./bin/armbian-config ...

Then in message_checkpoint.sh you can add something like this at the end:

# disable set_checkpoint if DEBUG has not been defined
[[ -n "$DEBUG" ]] || eval "set_checkpoint(){ :; }"

@igorpecovnik
Copy link
Member

IMHO it would be PITA to maintain two copies

Yes, I would also propose to have one.

@igorpecovnik igorpecovnik added 02 Milestone: First quarter release Needs review Seeking for review labels Feb 3, 2025
remove redundant file
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/large PR with 250 lines or more labels Feb 5, 2025
@github-actions github-actions bot added size/large PR with 250 lines or more and removed size/medium PR with more then 50 and less then 250 lines labels Feb 5, 2025
@github-actions github-actions bot added size/medium PR with more then 50 and less then 250 lines and removed size/large PR with 250 lines or more labels Feb 5, 2025
Copy link
Collaborator

@dimitry-ishenko dimitry-ishenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tearran I was under the impression that we only wanted timing info for debugging purposes. I kinda like that it's printed every time, but I don't know if there might be objections in terms of performance or some other reasons.

Comment on lines +15 to +26
# Conditional settings based on arguments
# for interactive vs non-interactive modes
if [[ "$1" == "--api" || "$1" == "--doc" ]]; then
UXMODE="false"
elif [[ -z "$1" || "$1" == "--cmd" || "$1" == "--help" ]]; then
UXMODE="true"
fi

# Additional check for interactive vs non-interactive mode
if [[ ! -t 0 ]]; then
UXMODE="false"
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW can be replaced with:

unset UXMODE
[[ -t 0 && "$1" =~ ^(|--cmd|--help)$ ]] && UXMODE="true"

Comment on lines +18 to +25
help)
echo "Usage: set_checkpoint <start|stop|mark|show> [description] [show]"
echo "Commands:"
echo " start Start the timer."
echo " stop Stop the timer."
echo " mark [description] [time] Mark a checkpoint with an optional description and an optional flag to show the output."
echo " show Show the total elapsed time and checkpoints."
;;
Copy link
Collaborator

@dimitry-ishenko dimitry-ishenko Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing indent? Same for other cases.

Comment on lines +53 to +57
local checkpoint_time=${set_checkpoint_CHECKPOINTS[$i]}
local checkpoint_duration=$((checkpoint_time - previous_time))
local description=${set_checkpoint_DESCRIPTIONS[$i]}
printf "%-30s: %d seconds\n" "${description:-Checkpoint $((i+1))}" "${checkpoint_duration}"
previous_time=$checkpoint_time
Copy link
Collaborator

@dimitry-ishenko dimitry-ishenko Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing indent inside for-loop?

Comment on lines +36 to +44
local checkpoint_time=$(date +%s)
local checkpoint_duration=$((checkpoint_time - set_checkpoint_PREV))
set_checkpoint_PREV=$checkpoint_time
set_checkpoint_CHECKPOINTS+=($checkpoint_time)
set_checkpoint_DESCRIPTIONS+=("$2")
local count=${#set_checkpoint_DESCRIPTIONS[@]}
if [[ "$3" == "true" && $UXMODE == "true" ]]; then
printf "%-30s %10d seconds\n" "$2:" "${checkpoint_duration}"
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but if you do this, you can avoid unnecessary calculations in non-UX mode:

	mark)
		if [[ "$UXMODE" == "true" ]]; then
			local checkpoint_time=$(date +%s)
			local checkpoint_duration=$((checkpoint_time - set_checkpoint_PREV))
			set_checkpoint_PREV=$checkpoint_time
			set_checkpoint_CHECKPOINTS+=($checkpoint_time)
			set_checkpoint_DESCRIPTIONS+=("$2")
			local count=${#set_checkpoint_DESCRIPTIONS[@]}
			[[ "$3" == "true" ]] && printf "%-30s %10d seconds\n" "$2:" "${checkpoint_duration}"
		fi
	;;

@Tearran
Copy link
Member Author

Tearran commented Feb 5, 2025

What about using an env var (eg, ARMBIAN_CONFIG_DEBUG or just DEBUG) to show the timing info?

@dimitry-ishenko I am not opposed to using a DEBUG environment variable in general. However, I would like to clarify that the messages displayed are not meant for debugging purposes and are not being introduced with this PR. These messages were previously implemented to improve the user experience by indicating that the application is working and not frozen.

In other words, this PR does not address bugs. Rather, it outlines the expected parsing load time limitations due to the linear nature of bash, such as function variable racing and other inherent constraints.

The addition of the timestamp is multifaceted. Firstly, IMHO it is important to recognize the limitations of bash, so that expected limitations are not identified as bugs. Showing loading time addresses this by clearly showing expected behavior. For example, 4 seconds may be undesirable but it is not a bug. By providing these user-facing messages, we aim to enhance user experience by transparently communicating the application's activity, thus avoiding any misconceptions of the application being unresponsive.

@dimitry-ishenko
Copy link
Collaborator

@dimitry-ishenko I am not opposed to using a DEBUG environment variable in general. However, I would like to clarify that the messages displayed are not meant for debugging purposes and are not being introduced with this PR. These messages were previously implemented to improve the user experience by indicating that the application is working and not frozen.

@Tearran sorry, not sure why I got the idea this was for debugging. Thanks for the clear explanation, it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
02 Milestone: First quarter release Needs review Seeking for review size/medium PR with more then 50 and less then 250 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants